Skip to content

Fix Codex title MCP startup and remote terminal spawning#618

Open
SmallSpider0 wants to merge 1 commit into
tiann:mainfrom
SmallSpider0:fix/codex-mcp-terminal
Open

Fix Codex title MCP startup and remote terminal spawning#618
SmallSpider0 wants to merge 1 commit into
tiann:mainfrom
SmallSpider0:fix/codex-mcp-terminal

Conversation

@SmallSpider0
Copy link
Copy Markdown
Contributor

Fix Codex title MCP startup and remote terminal spawning

Summary

  • Wrap the HAPI MCP stdio bridge with an inline Node proxy when launching it from Codex app-server.
  • Keep the proxy inline so compiled/release HAPI does not require an extra sidecar file.
  • Replace Bun terminal spawning in remote terminals with Node child_process.spawn, using /usr/bin/script on non-Darwin Unix when available.
  • Safely quote $SHELL when constructing the script -c command.

Motivation

Codex app-server can fail to initialize the Bun-backed HAPI MCP stdio bridge, leaving mcp__hapi__change_title unavailable in web sessions. Remote terminal sessions can also close immediately with Terminal stream closed unexpectedly in daemon/web environments.

Notes

  • HAPI_NODE_EXECUTABLE can override the Node executable used for the MCP stdio proxy.
  • Windows remote terminals remain unsupported, matching the existing behavior.
  • Terminal resize is best-effort in the new implementation; fidelity can vary depending on /usr/bin/script availability.

Validation

  • bun run --cwd cli typecheck
  • cd cli && bun x vitest run src/codex/utils/codexMcpConfig.test.ts src/codex/utils/appServerConfig.test.ts src/codex/codexLocal.test.ts src/codex/utils/appServerEventConverter.test.ts src/codex/utils/terminalEventGuard.test.ts — 79 tests passed
  • Codex app-server smoke: HAPI MCP startup reports HAPI_MCP ready using the generated inline proxy args
  • Terminal smoke: observed echo HAPI_TERMINAL_OK output and exit 0

Known local test limitation

bun run test was attempted, but this machine is Ubuntu 20.04 and the bundled cli/tools/unpacked/difft binary requires newer GLIBC versions (GLIBC_2.32, GLIBC_2.33, GLIBC_2.34). As a result, only src/modules/difftastic/index.test.ts fails locally; the remaining CLI tests run past that point and the targeted tests for this change pass.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Standalone installs now require an undeclared node executable for Codex MCP — the returned MCP server always runs node -e ..., while repo docs list Homebrew/prebuilt standalone installs and do not list Node as a runtime requirement for Codex/HAPI. On a standalone HAPI install with Codex available but no node on PATH, the HAPI MCP bridge fails before the child hapi mcp process can start. Evidence cli/src/codex/utils/buildHapiMcpBridge.ts:102.
    Suggested fix:

    const nodeCommand = resolveNodeExecutable();
    const hapiMcpServer = nodeCommand
        ? {
            command: nodeCommand,
            args: ['-e', HAPI_MCP_STDIO_PROXY_SCRIPT, bridgeCommand.command, ...bridgeCommand.args]
        }
        : bridgeCommand;
  • [Major] macOS remote terminals no longer get a PTY — resolveTerminalCommand() excludes Darwin from the script(1) path and then createPipeTerminal() spawns the shell with plain pipes, so interactive shells/apps have no controlling TTY on a documented supported platform. Evidence cli/src/terminal/TerminalManager.ts:84.
    Suggested fix:

    if (process.platform === 'darwin') {
        return this.createBunTerminal(terminalId, cols, rows, cwd, shell);
    }
    return this.createPipeTerminal(terminalId, cols, rows, cwd, shell);
  • [Major] Resize events are acknowledged but never applied to the running PTY — mutating the parent env object after spawn does not update the child process or the script(1) pseudo-terminal window size, so full-screen tools and line wrapping stay at the original dimensions after web/mobile resize. Evidence cli/src/terminal/TerminalManager.ts:214.
    Suggested fix:

    resize: (nextCols: number, nextRows: number) => {
        pty.resize(nextCols, nextRows);
    }

Summary
Review mode: initial
The diff introduces regressions in Codex MCP startup portability and remote terminal PTY behavior. Node availability is not documented as a HAPI runtime prerequisite, and the terminal replacement loses real PTY behavior on macOS plus resize propagation on Linux/script-backed sessions.

Testing
Not run (automation; review only, and PR code was not executed).

HAPI Bot

Comment thread cli/src/codex/utils/buildHapiMcpBridge.ts Outdated
Comment thread cli/src/terminal/TerminalManager.ts
Comment thread cli/src/terminal/TerminalManager.ts
@SmallSpider0 SmallSpider0 force-pushed the fix/codex-mcp-terminal branch from 44fe5f7 to 925001c Compare May 13, 2026 01:05
@SmallSpider0
Copy link
Copy Markdown
Contributor Author

SmallSpider0 commented May 13, 2026

Updated in 925001c to address the bot review:

  • The inline MCP proxy now falls back to the original HAPI MCP bridge if no Node executable is available; HAPI_NODE_EXECUTABLE remains supported as an explicit override.
  • macOS remote terminals now keep the Bun terminal path so they retain PTY behavior and resize support.
  • $SHELL is POSIX-quoted before use with script -c.

Revalidated locally:

  • bun run --cwd cli typecheck
  • targeted Codex/terminal vitest suite: 79 tests passed
  • generated bridge config smoke: Codex app-server reports HAPI_MCP ready
  • terminal smoke: echo HAPI_TERMINAL_OK, exit 0

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • [Major] Linux/script terminals ignore resize after creation — TerminalManager.resize() updates the runtime dimensions and calls terminal.resize(), but the new pipe-backed implementation only mutates the parent-side env object after the child has already spawned. On the default non-Darwin path with /usr/bin/script, the running PTY window size never changes, so full-screen tools and prompt wrapping stay at the original dimensions after web/mobile resize. Evidence cli/src/terminal/TerminalManager.ts:286.
    Suggested fix:
    import { spawn } from "node-pty";
    
    const ptyProcess = spawn(shell, ["-i"], {
        name: "xterm-256color",
        cols,
        rows,
        cwd,
        env
    });
    
    const terminal: TerminalHandle = {
        write: (data) => ptyProcess.write(data),
        resize: (nextCols, nextRows) => ptyProcess.resize(nextCols, nextRows),
        close: () => ptyProcess.kill()
    };

Summary

  • Review mode: follow-up after new commits
  • The prior Node executable and macOS PTY findings appear addressed in the latest diff. One resize regression remains in the default Linux/script terminal path.

Testing

  • Not run (automation; review only). Missing coverage: a TerminalManager test that verifies resize reaches the active PTY/process after creation.

HAPI Bot

Comment thread cli/src/terminal/TerminalManager.ts
@SmallSpider0 SmallSpider0 force-pushed the fix/codex-mcp-terminal branch from 925001c to 0d36b8d Compare May 13, 2026 01:10
@SmallSpider0
Copy link
Copy Markdown
Contributor Author

Updated again in 0d36b8d to address the follow-up resize finding:

  • The default non-Darwin terminal path now uses a small Python stdlib PTY bridge when python3/python is available.
  • Resize requests are forwarded to the active PTY with TIOCSWINSZ and SIGWINCH.
  • /usr/bin/script remains only a last-resort fallback when Python is unavailable.

Revalidated locally:

  • bun run --cwd cli typecheck
  • targeted Codex/terminal vitest suite: 79 tests passed
  • generated bridge config smoke: Codex app-server reports HAPI_MCP ready
  • terminal smoke: echo HAPI_TERMINAL_OK, exit 0
  • terminal resize smoke: after resizing to 100x30, stty size reports 30 100, exit 0

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Findings

  • No findings.

Summary

  • Review mode: follow-up after new commits
  • Reviewed the full latest diff plus the prior bot review context. The earlier Linux resize concern is addressed for the normal non-Darwin path by the added Python PTY bridge; the remaining pipe fallback is explicitly best-effort and only used when Python is unavailable or disabled.
  • Residual risk: no repo test currently exercises the new TerminalManager PTY bridge lifecycle/resize behavior.

Testing

  • Smoke-checked the Python PTY bridge locally for interactive startup, resize propagation via stty size, and foreground/background process cleanup.
  • Not run: bun run --cwd cli typecheck and targeted Vitest suite; bun is not installed in this runner.

HAPI Bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant