diff --git a/.env.example b/.env.example index da45c5c..85b58d7 100644 --- a/.env.example +++ b/.env.example @@ -1,6 +1,8 @@ -# Copy to .env and fill in your paths. +# Local-development reference only. The published `npx openroad-mcp` package +# does not load this file. PATH and ORFS_FLOW_PATH are inherited/auto-detected +# at startup; set these only to override an unusual install prefix. -# Directory containing the openroad binary (find with: dirname $(which openroad)) +# Directory containing the openroad binary (find with: dirname "$(command -v openroad)") PATH=/path/to/openroad/bin:$PATH # flow/ directory inside your ORFS checkout (only needed for report image tools) diff --git a/README.md b/README.md index c400049..72cc318 100644 --- a/README.md +++ b/README.md @@ -43,27 +43,9 @@ To use this MCP server, you need the server runtime, plus the underlying OpenROA For platform-specific Node.js and C++ toolchain setup instructions, see the **[Cross-Platform Build Guide](docs/CROSS_PLATFORM.md)**. -Before configuring your MCP client, you must provide the server with your local paths to OpenROAD and ORFS. +You do **not** need to clone this repo or pass path environment variables in the common case. The published `npx` package does not read a `.env` file. -### 1. Find Your Paths -Run these commands in your terminal to locate the necessary directories: - -```bash -# Get your OpenROAD binary directory: -dirname $(which openroad) - -# Check for the default ORFS flow directory: -ls ~/OpenROAD-flow-scripts/flow -# (If not found, search with: find ~ -maxdepth 4 -type d -name flow -path "*/OpenROAD-flow-scripts/*" 2>/dev/null) -``` - -### 2. Set Up Environment Variables -Copy the example environment file and fill in the paths you just found: - -```bash -cp .env.example .env -``` -Edit `.env` to ensure `PATH` includes your OpenROAD binary directory, and `ORFS_FLOW_PATH` points to your ORFS checkout. Pass these values into your MCP client's configuration via the `env` block. +On startup the server inherits the MCP client's environment, then fills `PATH` the same way `which openroad` would: current `PATH`, then your login-shell `PATH`, then common install locations (`/opt/homebrew/bin`, conda, local OpenROAD builds). `ORFS_FLOW_PATH` defaults to `~/OpenROAD-flow-scripts/flow`, and is also detected when ORFS sits next to the `openroad` binary. ## Supported MCP Clients @@ -76,16 +58,27 @@ Here is the standard base configuration used across most clients: } ``` -Find your specific client below for the exact configuration snippet and file location. - -*(Note: If your client supports environment variables in the config, inject your `PATH` and `ORFS_FLOW_PATH` directly in the snippet).* +Find your specific client below for the exact configuration snippet and file location.
Claude Code ```bash claude mcp add --transport stdio openroad-mcp -- npx -y openroad-mcp ``` -Or add the standard config to `.claude/settings.json`. + +Or add the standard config to `.mcp.json` / `.claude/settings.json`. + +If a GUI-launched client still cannot find `openroad`, pass an override. Use `command -v` so you do not hard-code paths: + +```bash +claude mcp add \ + --env PATH="$(dirname "$(command -v openroad)"):${PATH}" \ + --env ORFS_FLOW_PATH="${HOME}/OpenROAD-flow-scripts/flow" \ + --transport stdio openroad-mcp \ + -- npx -y openroad-mcp +``` + +Put `--transport` between `--env` and the server name so the CLI does not treat the name as another `KEY=value` pair.
Claude Desktop @@ -184,15 +177,17 @@ Once configured, your AI assistant will have access to the following tools. For ## Troubleshooting - **The server fails to start**: Ensure you have Node.js 22+. Older versions will fail. -- **Session creation fails**: Confirm `openroad` is on your `PATH`. The server spawns it by name; if it's missing, session creation (not startup) will fail. +- **Session creation fails**: Confirm `command -v openroad` works in a terminal. The server inherits PATH and searches common install locations; if your prefix is unusual, pass `PATH` with `--env` as shown in the Claude Code section. - **Commands rejected with CommandBlocked**: You sent a state-modifying command to `interactive_openroad_query`. Use `interactive_openroad_exec` instead. -- **Report images not found**: Make sure `ORFS_FLOW_PATH` points to your ORFS `flow/` directory. +- **Report images not found**: The server defaults to `~/OpenROAD-flow-scripts/flow`. If ORFS lives elsewhere, set `ORFS_FLOW_PATH` in the MCP client's `env` block (not a `.env` file). To get more detail, set `LOG_LEVEL=DEBUG` in the server's environment. ## Development -Clone the repository and run: +Clone the repository. `.env.example` is a local-dev reference only; copy it to `.env` if you use direnv or similar. The server still reads `process.env` (the MCP client's `env` block), not the file. + +Then run: ```bash cd typescript npm install diff --git a/docs/CROSS_PLATFORM.md b/docs/CROSS_PLATFORM.md index 39ca330..2b31a2f 100644 --- a/docs/CROSS_PLATFORM.md +++ b/docs/CROSS_PLATFORM.md @@ -67,7 +67,7 @@ xcode-select --install | Issue | Workaround | |-------|------------| -| OpenROAD is not on `PATH` after Homebrew install | Add `/opt/homebrew/bin` to your shell's `PATH`, or set `OPENROAD_ALLOWED_COMMANDS=openroad` and use the full path in your session config. | +| OpenROAD is not on `PATH` after Homebrew install | The server prepends `/opt/homebrew/bin` automatically. If session creation still fails, pass `PATH="$(dirname "$(command -v openroad)"):${PATH}"` in the MCP client `env` block. | | `node-pty` rebuild fails after a Node upgrade | `cd typescript && npm rebuild` | | `sharp` fails with "dyld: Library not loaded" | `cd typescript && npm rebuild --update-binary` | diff --git a/docs/SECURITY.md b/docs/SECURITY.md index a4c5c71..8a457b0 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -163,13 +163,15 @@ All variables are read at startup by [`typescript/src/config/settings.ts`](../ty | `OPENROAD_ALLOWED_COMMANDS` | string (comma-separated) | `openroad` | PTY spawn executable allowlist | | `OPENROAD_ENABLE_COMMAND_VALIDATION` | bool | `true` | Enables/disables `PtyHandler.validateCommand` | | `OPENROAD_WHITELIST_ENABLED` | bool | `true` | Enables/disables the Tcl command whitelist | -| `ORFS_FLOW_PATH` | path | `~/OpenROAD-flow-scripts/flow` | Root for ORFS reports; tilde-expanded at runtime | +| `ORFS_FLOW_PATH` | path | `~/OpenROAD-flow-scripts/flow` (auto-detected if unset) | Root for ORFS reports; tilde-expanded at runtime | | `LOG_LEVEL` | string | `INFO` | Root pino logger level (`DEBUG`, `INFO`, `WARNING`, `ERROR`, `CRITICAL`) | | `LOG_FORMAT` | string | (N/A) | Unused; logging uses pino with a fixed JSON format | CLI flags `--verbose` and `--log-level` override `LOG_LEVEL` after the settings are initialised. No other CLI flag overrides a Settings field. +`PATH` is not a Settings field. At startup the server inherits the client's `PATH`, then (only if `openroad` is not already found) merges the login-shell `PATH` and common install locations. An explicit `PATH` in the MCP client `env` block still wins when it already contains `openroad`. + --- ## Report Image Path Containment @@ -199,8 +201,9 @@ under the base directory. A symlink that points outside the base is caught here. **Additional constraints:** -- Only `.webp` files are served — the listing skips all other extensions and `read_report_image` - rejects any `image_name` that does not end in `.webp`. +- Only report-image extensions are served — `.webp`, `.png`, and the doubled `.webp.png` form + some ORFS builds emit. The listing skips everything else and `read_report_image` rejects any + `image_name` that does not end in one of them. - Symlinks are skipped during directory listing. - On-disk file size is capped at 50 MB before any decoding. - The base64-encoded payload is targeted at 15 KB; larger images are downscaled using sharp diff --git a/typescript/README.md b/typescript/README.md index 232d86a..3d01d53 100644 --- a/typescript/README.md +++ b/typescript/README.md @@ -82,7 +82,7 @@ The server reads configuration from environment variables. Key variables: |---|---|---| | `OPENROAD_MAX_SESSIONS` | `50` | Maximum concurrent sessions | | `OPENROAD_COMMAND_TIMEOUT` | `30.0` | Default per-command timeout in seconds | -| `ORFS_FLOW_PATH` | `~/OpenROAD-flow-scripts/flow` | Path to ORFS flow directory | +| `ORFS_FLOW_PATH` | `~/OpenROAD-flow-scripts/flow` (auto-detected if unset) | Path to ORFS flow directory | | `OPENROAD_WHITELIST_ENABLED` | `true` | Enable Tcl command whitelist | Full list: [docs/SECURITY.md#environment-variable-reference](https://github.com/The-OpenROAD-Project/openroad-mcp/blob/main/docs/SECURITY.md#environment-variable-reference) diff --git a/typescript/__tests__/config/path_env.test.ts b/typescript/__tests__/config/path_env.test.ts new file mode 100644 index 0000000..ab54101 --- /dev/null +++ b/typescript/__tests__/config/path_env.test.ts @@ -0,0 +1,184 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { + mergePathDirs, + findExecutable, + parseLoginShellPath, + enrichInheritedEnv, + defaultExtraBinDirs, + defaultOrfsFlowCandidates, + type PathEnvDeps, +} from "../../src/config/path_env.js"; + +function makeExecutable(dir: string, name: string): string { + fs.mkdirSync(dir, { recursive: true }); + const filePath = path.join(dir, name); + fs.writeFileSync(filePath, "#!/bin/sh\n"); + fs.chmodSync(filePath, 0o755); + return filePath; +} + +function makeOrfsFlow(root: string): string { + const flow = path.join(root, "flow"); + fs.mkdirSync(path.join(flow, "designs"), { recursive: true }); + fs.mkdirSync(path.join(flow, "platforms"), { recursive: true }); + return flow; +} + +describe("path_env", () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openroad-path-env-")); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + describe("mergePathDirs", () => { + it("deduplicates and skips empty segments while preserving first-seen order", () => { + expect(mergePathDirs( + ["/a", "/b"].join(path.delimiter), + ["/b", "/c"], + "", + ` ${path.delimiter}/a`, + )).toBe(["/a", "/b", "/c"].join(path.delimiter)); + }); + }); + + describe("findExecutable", () => { + it("returns the first executable named on PATH", () => { + const bin = path.join(tmpDir, "bin"); + const openroad = makeExecutable(bin, "openroad"); + expect(findExecutable("openroad", ["/usr/bin", bin].join(path.delimiter))).toBe(openroad); + }); + + it("returns undefined when the binary is missing", () => { + expect(findExecutable("openroad", "/usr/bin")).toBeUndefined(); + }); + }); + + describe("parseLoginShellPath", () => { + it("reads the marker even when the shell printed a banner first", () => { + expect(parseLoginShellPath("Welcome!\n__OPENROAD_MCP_PATH__=/opt/homebrew/bin:/usr/bin\n")).toBe( + "/opt/homebrew/bin:/usr/bin", + ); + }); + + it("returns undefined when the marker is absent", () => { + expect(parseLoginShellPath("/opt/homebrew/bin")).toBeUndefined(); + }); + }); + + describe("default location lists", () => { + it("puts Homebrew first on macOS", () => { + expect(defaultExtraBinDirs("/Users/me", "darwin")[0]).toBe("/opt/homebrew/bin"); + }); + + it("includes the default ORFS checkout", () => { + expect(defaultOrfsFlowCandidates("/home/me")[0]).toBe( + path.join("/home/me", "OpenROAD-flow-scripts", "flow"), + ); + }); + }); + + describe("enrichInheritedEnv", () => { + function deps(overrides: Partial & { env: NodeJS.ProcessEnv }): PathEnvDeps { + return { + isExecutable: (p) => { + try { + fs.accessSync(p, fs.constants.X_OK); + return fs.statSync(p).isFile(); + } catch { + return false; + } + }, + dirExists: (p) => { + try { + return fs.statSync(p).isDirectory(); + } catch { + return false; + } + }, + extraBinDirs: () => [], + extraOrfsDirs: () => [], + readLoginShellPath: () => undefined, + ...overrides, + }; + } + + it("leaves PATH unchanged when openroad is already on it", () => { + const bin = path.join(tmpDir, "already"); + makeExecutable(bin, "openroad"); + const current = [bin, "/usr/bin"].join(path.delimiter); + const env: NodeJS.ProcessEnv = { PATH: current }; + let loginCalled = false; + const result = enrichInheritedEnv( + deps({ + env, + extraBinDirs: () => ["/opt/homebrew/bin"], + readLoginShellPath: () => { + loginCalled = true; + return "/opt/homebrew/bin"; + }, + }), + ); + expect(env.PATH).toBe(current); + expect(result.openroad).toBe(path.join(bin, "openroad")); + expect(loginCalled).toBe(false); + }); + + it("prepends the login-shell directory that contains openroad", () => { + const loginBin = path.join(tmpDir, "login-bin"); + makeExecutable(loginBin, "openroad"); + const env: NodeJS.ProcessEnv = { PATH: "/usr/bin:/bin" }; + const result = enrichInheritedEnv( + deps({ + env, + readLoginShellPath: () => [loginBin, "/usr/bin"].join(path.delimiter), + }), + ); + expect(result.openroad).toBe(path.join(loginBin, "openroad")); + expect(env.PATH?.split(path.delimiter)[0]).toBe(loginBin); + }); + + it("finds openroad in extra bin dirs used by Homebrew/conda/local builds", () => { + const brewBin = path.join(tmpDir, "homebrew"); + makeExecutable(brewBin, "openroad"); + const env: NodeJS.ProcessEnv = { PATH: "/usr/bin" }; + const result = enrichInheritedEnv(deps({ env, extraBinDirs: () => [brewBin] })); + expect(result.openroad).toBe(path.join(brewBin, "openroad")); + expect(env.PATH?.split(path.delimiter)[0]).toBe(brewBin); + }); + + it("does not overwrite an explicit ORFS_FLOW_PATH", () => { + const env: NodeJS.ProcessEnv = { PATH: "/usr/bin", ORFS_FLOW_PATH: "/custom/flow" }; + const flow = makeOrfsFlow(path.join(tmpDir, "OpenROAD-flow-scripts")); + const result = enrichInheritedEnv(deps({ env, extraOrfsDirs: () => [flow] })); + expect(result.orfsFlowPath).toBe("/custom/flow"); + expect(env.ORFS_FLOW_PATH).toBe("/custom/flow"); + }); + + it("sets ORFS_FLOW_PATH when a candidate flow tree exists", () => { + const flow = makeOrfsFlow(path.join(tmpDir, "OpenROAD-flow-scripts")); + const env: NodeJS.ProcessEnv = { PATH: "/usr/bin" }; + const result = enrichInheritedEnv(deps({ env, extraOrfsDirs: () => [flow] })); + expect(result.orfsFlowPath).toBe(flow); + expect(env.ORFS_FLOW_PATH).toBe(flow); + }); + + it("detects ORFS next to a local openroad install", () => { + const root = path.join(tmpDir, "OpenROAD-flow-scripts"); + const flow = makeOrfsFlow(root); + const bin = path.join(root, "tools", "install", "OpenROAD", "bin"); + makeExecutable(bin, "openroad"); + const env: NodeJS.ProcessEnv = { PATH: bin }; + const result = enrichInheritedEnv(deps({ env, extraOrfsDirs: () => [] })); + expect(result.orfsFlowPath).toBe(flow); + expect(env.ORFS_FLOW_PATH).toBe(flow); + }); + }); +}); diff --git a/typescript/__tests__/core/manager.test.ts b/typescript/__tests__/core/manager.test.ts index fe8cac3..fea0fe6 100644 --- a/typescript/__tests__/core/manager.test.ts +++ b/typescript/__tests__/core/manager.test.ts @@ -19,8 +19,10 @@ interface MockSession { terminatedAt: Date | null; checkAlive: Mock; start: Mock; + verifyResponsive: Mock; sendCommand: Mock; readOutput: Mock; + runCommand: Mock; getInfo: Mock; getDetailedMetrics: Mock; getCommandHistory: Mock; @@ -53,6 +55,7 @@ function makeMockSession(sessionId: string, alive = true): MockSession { terminatedAt: null, checkAlive: vi.fn().mockReturnValue(alive), start: vi.fn().mockResolvedValue(undefined), + verifyResponsive: vi.fn().mockResolvedValue(undefined), sendCommand: vi.fn().mockResolvedValue(undefined), readOutput: vi.fn().mockResolvedValue({ output: "ok", @@ -63,6 +66,15 @@ function makeMockSession(sessionId: string, alive = true): MockSession { bufferSize: 0, error: null, }), + runCommand: vi.fn().mockResolvedValue({ + output: "ok", + sessionId, + timestamp: new Date().toISOString(), + executionTime: 0.01, + commandCount: 1, + bufferSize: 0, + error: null, + }), getInfo: vi.fn().mockResolvedValue({ sessionId, createdAt: new Date().toISOString(), @@ -85,6 +97,25 @@ function makeMockSession(sessionId: string, alive = true): MockSession { const MockedSession = vi.mocked(InteractiveSession); +/** Park the next session's handshake so a create stays in flight on demand. + * Must be a plain function, not an arrow, so `new` still works. */ +function stallNextHandshake(): { handshakeStarted: Promise; release: () => void } { + let release!: () => void; + let signalStarted!: () => void; + const handshakeStarted = new Promise((r) => (signalStarted = r)); + + MockedSession.mockImplementationOnce(function (this: unknown, sessionId: string) { + const mock = makeMockSession(sessionId); + mock.verifyResponsive = vi.fn().mockImplementation(async () => { + signalStarted(); + await new Promise((r) => (release = r)); + }); + return mock as unknown as InteractiveSession; + } as unknown as (sessionId: string) => InteractiveSession); + + return { handshakeStarted, release: () => release() }; +} + describe("OpenROADManager", () => { let manager: OpenROADManager; let created: MockSession[]; @@ -133,6 +164,40 @@ describe("OpenROADManager", () => { await expect(limited.createSession({ sessionId: "s2" })).rejects.toThrow(/Maximum session limit/); }); + it("does not hold the manager lock across a slow startup handshake", async () => { + const { handshakeStarted, release } = stallNextHandshake(); + + const slow = manager.createSession({ sessionId: "slow" }); + await handshakeStarted; + + // A second session stuck in its handshake must not stall unrelated + // lifecycle work; before, this awaited the full handshake timeout. + await expect( + Promise.race([ + manager.listSessions().then(() => "listed"), + new Promise((r) => setTimeout(() => r("blocked"), 1000)), + ]), + ).resolves.toBe("listed"); + + release(); + await slow; + }); + + it("counts in-flight reservations towards the session cap", async () => { + const limited = new OpenROADManager(1); + const { handshakeStarted, release } = stallNextHandshake(); + + const first = limited.createSession({ sessionId: "s1" }); + await handshakeStarted; + + // Startup happens outside the lock now, so the reserved-but-unspawned + // slot is the only thing keeping concurrent creates under the cap. + await expect(limited.createSession({ sessionId: "s2" })).rejects.toThrow(/Maximum session limit/); + + release(); + await first; + }); + it("falls back to the default buffer size when bufferSize is 0", async () => { await manager.createSession({ sessionId: "zero", bufferSize: 0 }); // InteractiveSession is constructed with (sessionId, bufferSize); a 0 must @@ -155,11 +220,13 @@ describe("OpenROADManager", () => { }); describe("executeCommand", () => { - it("delegates to sendCommand then readOutput", async () => { + it("delegates to runCommand so completion is actually detected", async () => { await manager.createSession({ sessionId: "s1" }); const result = await manager.executeCommand("s1", "report_wns"); - expect(created[0]!.sendCommand).toHaveBeenCalledWith("report_wns"); - expect(created[0]!.readOutput).toHaveBeenCalledOnce(); + expect(created[0]!.runCommand).toHaveBeenCalledWith("report_wns", expect.any(Number)); + // readOutput cannot tell a finished command from a quiet one, so the + // command path must never fall back to it. + expect(created[0]!.readOutput).not.toHaveBeenCalled(); expect(result.output).toBe("ok"); }); @@ -172,8 +239,8 @@ describe("OpenROADManager", () => { it("falls back to the default timeout when timeoutMs is 0", async () => { await manager.createSession({ sessionId: "s1" }); await manager.executeCommand("s1", "report_wns", 0); - // 0 must not be forwarded as an instant timeout; readOutput gets the default. - const timeoutArg = created[0]!.readOutput.mock.calls[0]![0] as number; + // 0 must not be forwarded as an instant timeout; runCommand gets the default. + const timeoutArg = created[0]!.runCommand.mock.calls[0]![1] as number; expect(timeoutArg).toBeGreaterThan(0); }); }); @@ -323,7 +390,7 @@ describe("OpenROADManager", () => { it("forwards an explicit positive timeoutMs as-is", async () => { await manager.createSession({ sessionId: "s1" }); await manager.executeCommand("s1", "report_wns", 5000); - expect(created[0]!.readOutput).toHaveBeenCalledWith(5000); + expect(created[0]!.runCommand).toHaveBeenCalledWith("report_wns", 5000); }); }); diff --git a/typescript/__tests__/interactive/pty_handler.test.ts b/typescript/__tests__/interactive/pty_handler.test.ts index a2be8f4..c8f9bb3 100644 --- a/typescript/__tests__/interactive/pty_handler.test.ts +++ b/typescript/__tests__/interactive/pty_handler.test.ts @@ -79,9 +79,36 @@ describe("PtyHandler", () => { const opts = call[2]!; expect(opts.cwd).toBe("/tmp"); expect((opts.env as Record)["TEST"]).toBe("value"); - expect((opts.env as Record)["TERM"]).toBe("xterm-256color"); - expect((opts.env as Record)["COLUMNS"]).toBe("80"); + // A plain terminal and a width beyond any realistic command keep + // OpenROAD's line editor from horizontally scrolling long input, which + // would otherwise redraw the whole line once per character. + expect((opts.env as Record)["TERM"]).toBe("dumb"); + expect((opts.env as Record)["COLUMNS"]).toBe("4096"); expect((opts.env as Record)["LINES"]).toBe("24"); + expect(opts.cols).toBe(4096); + }); + + it("lets an explicit caller env override the terminal defaults", async () => { + await handler.createSession(["echo", "hello"], { TERM: "vt100", COLUMNS: "120" }); + + const opts = vi.mocked(spawn).mock.calls[0]![2]!; + expect((opts.env as Record)["TERM"]).toBe("vt100"); + expect(opts.name).toBe("vt100"); + expect(opts.cols).toBe(120); + }); + + it.each([ + ["zero", "0"], + ["negative", "-5"], + ["fractional", "80.5"], + ["non-numeric", "wide"], + ["blank", ""], + ])("falls back to the default geometry for a %s COLUMNS", async (_label, columns) => { + // node-pty cannot honour any of these as a cell count, so a bad value + // must not reach it -- least of all a negative, which is truthy. + await handler.createSession(["echo", "hello"], { COLUMNS: columns }); + + expect(vi.mocked(spawn).mock.calls[0]![2]!.cols).toBe(4096); }); it("marks process as alive after createSession", async () => { diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index 162d041..523fe37 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -124,15 +124,38 @@ describe("InteractiveSession", () => { expect(session.inputQueueSize()).toBe(1); }); - it("appends newline to command if missing", async () => { - session.state = SessionState.ACTIVE; + it("terminates each command with a carriage return, normalising any trailing newline", async () => { + await session.start(["openroad", "-no_init"]); (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); await session.sendCommand("test command"); - expect(session.inputQueueSize()).toBe(1); - await session.sendCommand("with newline\n"); expect(session.commandCount).toBe(2); + + // A line editor in raw mode accepts CR, not LF; sending LF can leave the + // command unsubmitted in the edit buffer. + await vi.waitFor(() => { + expect(mockPty.writeInput).toHaveBeenCalledTimes(2); + }); + const written = (mockPty.writeInput as ReturnType).mock.calls.map((c) => c[0]); + expect(written[0]).toBe("test command\r"); + expect(written[1]).toBe("with newline\r"); + }); + + it("converts embedded newlines to carriage returns in a multi-line command", async () => { + await session.start(["openroad", "-no_init"]); + (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); + + // Each inner line has to be accepted by the editor too, so an LF left in + // the middle would strand everything after it in the edit buffer. + await session.sendCommand("set a 1\nset b 2"); + + await vi.waitFor(() => { + expect(mockPty.writeInput).toHaveBeenCalledTimes(1); + }); + expect((mockPty.writeInput as ReturnType).mock.calls[0]![0]).toBe( + "set a 1\rset b 2\r", + ); }); it("throws SessionTerminatedError on terminated session", async () => { @@ -253,6 +276,167 @@ describe("InteractiveSession", () => { expect(result.output).toContain("delayed output"); expect(result.executionTime).toBeGreaterThan(0); }); + + it("cannot detect completion: returns the echo as success while the command is still running", async () => { + // Characterisation test, not aspirational. readOutput stops after + // MAX_COMMAND_COMPLETION_WINDOW of silence, so a command that echoes + // immediately and then thinks for 400ms looks finished and successful. + // This is why executeCommand routes through runCommand instead; if you + // are tempted to use readOutput for a command, read this first. + await session.outputBuffer.append("read_db /tmp/big.odb\r\n"); // PTY echo + setTimeout(() => { + void session.outputBuffer.append("real result arrives late\r\n"); + }, 400); + + const result = await session.readOutput(5000); + + expect(result.output).not.toContain("real result arrives late"); + expect(result.error).toBeNull(); + expect(result.executionTime).toBeLessThan(0.3); + }); + }); + + describe("runCommand (sentinel-based completion)", () => { + const NONCE_RE = /join \{ORMCP DONE (\S+)\}/; + + /** + * Drive the mock PTY like a real one: echo every written line back, then + * after `thinkMs` of silence emit the command's output followed by the + * sentinel. `thinkMs` is the whole point -- it reproduces a command that + * stays quiet longer than the old 100ms completion window. + */ + function wirePty(opts: { thinkMs: number; output: string; emitSentinel?: boolean }): void { + const { thinkMs, output, emitSentinel = true } = opts; + (mockPty.writeInput as ReturnType).mockImplementation((data: string) => { + // A terminal echoes the submitted line back as CRLF. + void session.outputBuffer.append(data.replace(/\r$/, "\r\n")); + const match = NONCE_RE.exec(data); + if (!match) return; + const nonce = match[1]!; + setTimeout(() => { + void session.outputBuffer.append(output); + if (emitSentinel) void session.outputBuffer.append(`ORMCP-DONE-${nonce}\r\n`); + }, thinkMs); + }); + } + + beforeEach(async () => { + await session.start(["openroad", "-no_init"]); + }); + + it("waits for a command that stays silent well past the 100ms completion window", async () => { + // The regression that shipped: readOutput() saw the PTY echo, waited + // MAX_COMMAND_COMPLETION_WINDOW (100ms), saw nothing more, and returned + // the echo as a successful empty result while OpenROAD was still working. + wirePty({ thinkMs: 400, output: "wns max -0.485\r\n" }); + + const result = await session.runCommand("report_wns", 5000); + + expect(result.output).toContain("wns max -0.485"); + expect(result.error).toBeNull(); + expect(result.executionTime).toBeGreaterThan(0.3); + }); + + it("strips the sentinel echo and marker from the returned output", async () => { + wirePty({ thinkMs: 10, output: "real output\r\n" }); + + const result = await session.runCommand("report_wns", 2000); + + expect(result.output).toContain("real output"); + expect(result.output).not.toContain("ORMCP"); + expect(result.output).not.toContain("join {"); + }); + + it("reports CommandTimeout instead of silent success when the command never finishes", async () => { + wirePty({ thinkMs: 10, output: "partial work\r\n", emitSentinel: false }); + + const result = await session.runCommand("repair_timing", 400); + + expect(result.error).toMatch(/CommandTimeout/); + expect(result.output).toContain("partial work"); + }); + + it("does not report success when the process dies mid-command", async () => { + (mockPty.writeInput as ReturnType).mockImplementation((data: string) => { + void session.outputBuffer.append(data.replace(/\r$/, "\r\n")); + if (NONCE_RE.test(data)) { + setTimeout(() => { + (mockPty.isProcessAlive as ReturnType).mockReturnValue(false); + }, 50); + } + }); + + const result = await session.runCommand("read_db /tmp/huge.odb", 3000); + + expect(result.error).toMatch(/SessionTerminated/); + }); + + it("keeps the sentinel out of the audit trail", async () => { + wirePty({ thinkMs: 10, output: "ok\r\n" }); + + await session.runCommand("report_tns", 2000); + + const history = session.getCommandHistory(); + expect(history).toHaveLength(1); + expect(history[0]!.command).toBe("report_tns"); + expect(session.commandCount).toBe(1); + }); + + it("records a real per-command execution time rather than a fixed window", async () => { + wirePty({ thinkMs: 350, output: "done\r\n" }); + + await session.runCommand("place_design", 5000); + + const entry = session.getCommandHistory()[0]!; + expect(entry.executionTime).toBeGreaterThan(0.3); + }); + + it("suppresses a stale marker left behind by an earlier timed-out command", async () => { + await session.outputBuffer.append("ORMCP-DONE-staleabc123\r\n"); + wirePty({ thinkMs: 10, output: "fresh output\r\n" }); + + const result = await session.runCommand("report_checks", 2000); + + expect(result.output).toContain("fresh output"); + expect(result.output).not.toContain("staleabc123"); + }); + + it("verifyResponsive rejects a session whose line editor never accepts input", async () => { + // The VM failure: the process is alive and echoes every submitted line + // back cleanly, but never runs it. The echo alone must not satisfy the + // probe, which is why the probe token is assembled at runtime. + (mockPty.writeInput as ReturnType).mockImplementation((data: string) => { + void session.outputBuffer.append(data.replace(/\r$/, "\r\n")); + }); + + await expect(session.verifyResponsive(600)).rejects.toThrow(/not executing commands/); + }); + + it("verifyResponsive accepts a healthy session without polluting the audit trail", async () => { + wirePty({ thinkMs: 10, output: "ORMCP-READY-OK\r\n" }); + + await session.verifyResponsive(2000); + + expect(session.getCommandHistory()).toHaveLength(0); + expect(session.commandCount).toBe(0); + }); + + it("reports a timeout even when the partial output contains an error pattern", async () => { + wirePty({ thinkMs: 10, output: "Error: something went wrong\r\n", emitSentinel: false }); + + const result = await session.runCommand("long_running_thing", 400); + + // A truncated result must not be presented as a completed, failed one. + expect(result.error).toMatch(/CommandTimeout/); + }); + + it("still surfaces OpenROAD errors detected in the output", async () => { + wirePty({ thinkMs: 10, output: 'invalid command name "bogus_cmd"\r\n' }); + + const result = await session.runCommand("bogus_cmd", 2000); + + expect(result.error).toContain("Invalid command"); + }); }); describe("checkAlive", () => { @@ -577,6 +761,27 @@ describe("InteractiveSession", () => { expect(result.error).toMatch(/Fatal error/); }); + it("detects OpenROAD [ERROR CODE-NNNN] lines, which have no colon", async () => { + // The dry-run failure: repair_timing printed [ERROR RSZ-0089] and the + // tool still returned error: null because only "Error:" / "ERROR:" were + // recognised. Warnings must stay non-errors. + await session.outputBuffer.append( + "[WARNING EST-0027] no estimated parasitics. Using wire load models.\n" + + "[ERROR RSZ-0089] Could not find a resistance value for any corner. Cannot evaluate max wire length for buffer.\n", + ); + const result = await session.readOutput(100); + expect(result.error).toMatch(/OpenROAD RSZ-0089/); + expect(result.error).toMatch(/resistance value/); + }); + + it("does not treat an OpenROAD [WARNING] line as a command failure", async () => { + await session.outputBuffer.append( + "[WARNING EST-0027] no estimated parasitics. Using wire load models.\n", + ); + const result = await session.readOutput(100); + expect(result.error).toBeNull(); + }); + it("detects invalid command name pattern", async () => { await session.outputBuffer.append('invalid command name "foo_bar"\n'); const result = await session.readOutput(100); diff --git a/typescript/__tests__/performance/benchmarks.test.ts b/typescript/__tests__/performance/benchmarks.test.ts index ece730b..6232901 100644 --- a/typescript/__tests__/performance/benchmarks.test.ts +++ b/typescript/__tests__/performance/benchmarks.test.ts @@ -13,8 +13,8 @@ interface MockSession { lastActivity: Date; checkAlive: Mock; start: Mock; - sendCommand: Mock; - readOutput: Mock; + verifyResponsive: Mock; + runCommand: Mock; getInfo: Mock; getDetailedMetrics: Mock; getCommandHistory: Mock; @@ -43,8 +43,8 @@ function makeMockSession(sessionId: string): MockSession { lastActivity: new Date(), checkAlive: vi.fn().mockReturnValue(true), start: vi.fn().mockResolvedValue(undefined), - sendCommand: vi.fn().mockResolvedValue(undefined), - readOutput: vi.fn().mockResolvedValue({ + verifyResponsive: vi.fn().mockResolvedValue(undefined), + runCommand: vi.fn().mockResolvedValue({ output: "ok", sessionId, timestamp: new Date().toISOString(), @@ -245,7 +245,7 @@ describe("Stress Tests", () => { } const session = created[0]!; - expect(session.readOutput.mock.calls.length).toBe(TOTAL); + expect(session.runCommand.mock.calls.length).toBe(TOTAL); expect(session.checkAlive()).toBe(true); }); diff --git a/typescript/__tests__/performance/memory_monitoring.test.ts b/typescript/__tests__/performance/memory_monitoring.test.ts index 266a162..3e73267 100644 --- a/typescript/__tests__/performance/memory_monitoring.test.ts +++ b/typescript/__tests__/performance/memory_monitoring.test.ts @@ -12,8 +12,8 @@ function makeMockSession(sessionId: string) { lastActivity: new Date(), checkAlive: vi.fn().mockReturnValue(true), start: vi.fn().mockResolvedValue(undefined), - sendCommand: vi.fn().mockResolvedValue(undefined), - readOutput: vi.fn().mockResolvedValue({ + verifyResponsive: vi.fn().mockResolvedValue(undefined), + runCommand: vi.fn().mockResolvedValue({ output: "ok", sessionId, timestamp: new Date().toISOString(), diff --git a/typescript/__tests__/performance/response_sizes.test.ts b/typescript/__tests__/performance/response_sizes.test.ts index efe6c95..0b9d34c 100644 --- a/typescript/__tests__/performance/response_sizes.test.ts +++ b/typescript/__tests__/performance/response_sizes.test.ts @@ -114,6 +114,7 @@ describe("Live Tool Compactness", () => { lastActivity: new Date(), checkAlive: vi.fn().mockReturnValue(true), start: vi.fn().mockResolvedValue(undefined), + verifyResponsive: vi.fn().mockResolvedValue(undefined), getInfo: vi.fn().mockResolvedValue({ sessionId, createdAt: new Date().toISOString(), diff --git a/typescript/__tests__/tools/report_images.test.ts b/typescript/__tests__/tools/report_images.test.ts index 431c6ca..70e2965 100644 --- a/typescript/__tests__/tools/report_images.test.ts +++ b/typescript/__tests__/tools/report_images.test.ts @@ -77,6 +77,16 @@ async function writeRealWebp(filePath: string, width: number, height: number): P fs.writeFileSync(filePath, buffer); } +/** Writes a real PNG, for the `.webp.png` files some ORFS builds emit. */ +async function writeRealPng(filePath: string, width: number, height: number): Promise { + const buffer = await sharp({ + create: { width, height, channels: 3, background: { r: 10, g: 20, b: 30 } }, + }) + .png() + .toBuffer(); + fs.writeFileSync(filePath, buffer); +} + beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openroad-test-")); }); @@ -108,6 +118,12 @@ describe("classifyImageType", () => { const [stage, _type] = classifyImageType("nounderscore.webp"); expect(stage).toBe("unknown"); }); + + it("classifies the doubled .webp.png extension some ORFS builds emit", () => { + expect(classifyImageType("final_all.webp.png")).toEqual(["final", "complete_design"]); + expect(classifyImageType("final_routing.webp.png")).toEqual(["final", "routing_visualization"]); + expect(classifyImageType("cts_clk.png")).toEqual(["cts", "clock_visualization"]); + }); }); describe("validatePlatformDesign", () => { @@ -175,6 +191,46 @@ describe("ListReportImagesTool", () => { expect(result.images_by_stage).toEqual({}); }); + it("finds images written with the doubled .webp.png extension", async () => { + // The naming a real ORFS build produced; matching only ".webp" reported + // zero images for a directory that was full of them. + const { flowPath } = createFixture("nangate45", "gcd", "run-123", [ + "final_all.webp.png", + "final_routing.webp.png", + "cts_clk.webp", + ]); + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath, + WHITELIST_ENABLED: false, + }); + const raw = await tool.execute("nangate45", "gcd", "run-123"); + const result = JSON.parse(raw); + expect(result.total_images).toBe(3); + expect(result.images_by_stage["final"]).toHaveLength(2); + expect(result.images_by_stage["cts"]).toHaveLength(1); + expect(result.images_by_stage["final"][0].type).toBe("complete_design"); + }); + + it("explains an empty result when the directory holds unrecognised image files", async () => { + const { flowPath, runPath } = createFixture("nangate45", "gcd", "run-odd", []); + fs.writeFileSync(path.join(runPath, "layout.tiff"), Buffer.from("x")); + fs.writeFileSync(path.join(runPath, "shot.jpeg"), Buffer.from("x")); + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath, + WHITELIST_ENABLED: false, + }); + const raw = await tool.execute("nangate45", "gcd", "run-odd"); + const result = JSON.parse(raw); + expect(result.total_images).toBe(0); + // A bare zero cannot distinguish "no images" from "tool did not match them". + expect(result.message).toMatch(/do not match the expected extensions/); + expect(result.message).toContain("shot.jpeg"); + }); + it("lists all .webp files grouped by stage", async () => { const { flowPath } = createFixture(); (getSettings as ReturnType).mockReturnValue({ @@ -301,6 +357,27 @@ describe("ReadReportImageTool", () => { expect(result.metadata.filename).toBe("cts_clk.webp"); expect(result.metadata.stage).toBe("cts"); expect(result.metadata.type).toBe("clock_visualization"); + expect(result.metadata.format).toBe("webp"); + }); + + it("reports the real format of a .webp.png, which is returned as PNG bytes", async () => { + const { flowPath, runPath } = createFixture("nangate45", "gcd", "run-123", []); + // Small enough to skip compression, so the bytes are the file's own. + await writeRealPng(path.join(runPath, "final_all.webp.png"), 4, 4); + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath, + WHITELIST_ENABLED: false, + }); + + const result = JSON.parse(await tool.execute("nangate45", "gcd", "run-123", "final_all.webp.png")); + + // The declared format has to match the bytes: a consumer that trusts it + // instead of sniffing the header would otherwise fail to decode. + const decoded = Buffer.from(result.image_data, "base64"); + expect(decoded.subarray(0, 4)).toEqual(Buffer.from([0x89, 0x50, 0x4e, 0x47])); + expect(result.metadata.format).toBe("png"); }); it("returns FileTooLarge error when image exceeds 50 MB", async () => { @@ -324,7 +401,7 @@ describe("ReadReportImageTool", () => { statSpy.mockRestore(); }); - it("rejects non-.webp extension", async () => { + it("rejects a non-image extension", async () => { const { flowPath } = createFixture(); (getSettings as ReturnType).mockReturnValue({ platforms: ["nangate45"], @@ -332,7 +409,7 @@ describe("ReadReportImageTool", () => { flowPath, WHITELIST_ENABLED: false, }); - const raw = await tool.execute("nangate45", "gcd", "run-123", "cts_clk.png"); + const raw = await tool.execute("nangate45", "gcd", "run-123", "cts_clk.txt"); const result = JSON.parse(raw); expect(result.error).toBe("InvalidImageName"); }); diff --git a/typescript/src/config/path_env.ts b/typescript/src/config/path_env.ts new file mode 100644 index 0000000..2abf9dd --- /dev/null +++ b/typescript/src/config/path_env.ts @@ -0,0 +1,186 @@ +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const LOGIN_SHELL_TIMEOUT_MS = 2500; +const LOGIN_SHELL_PATH_MARKER = "__OPENROAD_MCP_PATH__="; + +export type PathEnvDeps = { + env: NodeJS.ProcessEnv; + isExecutable: (filePath: string) => boolean; + dirExists: (dirPath: string) => boolean; + extraBinDirs: () => string[]; + extraOrfsDirs: () => string[]; + readLoginShellPath: () => string | undefined; +}; + +export function isExecutableFile(filePath: string): boolean { + try { + fs.accessSync(filePath, fs.constants.X_OK); + return fs.statSync(filePath).isFile(); + } catch { + return false; + } +} + +export function dirExists(dirPath: string): boolean { + try { + return fs.statSync(dirPath).isDirectory(); + } catch { + return false; + } +} + +export function defaultExtraBinDirs(home: string, platform: NodeJS.Platform): string[] { + const dirs = [ + path.join(home, ".local", "bin"), + path.join(home, "OpenROAD", "build", "src"), + path.join(home, "OpenROAD-flow-scripts", "tools", "install", "OpenROAD", "bin"), + path.join(home, "miniconda3", "bin"), + path.join(home, "anaconda3", "bin"), + path.join(home, "miniforge3", "bin"), + path.join(home, "mambaforge", "bin"), + "/opt/conda/bin", + ]; + if (platform === "darwin") { + dirs.unshift("/opt/homebrew/bin", "/usr/local/bin"); + } else if (platform !== "win32") { + dirs.unshift("/usr/local/bin"); + } + return dirs; +} + +export function defaultOrfsFlowCandidates(home: string): string[] { + return [ + path.join(home, "OpenROAD-flow-scripts", "flow"), + path.join(home, "src", "OpenROAD-flow-scripts", "flow"), + path.join(home, "tools", "OpenROAD-flow-scripts", "flow"), + ]; +} + +export function mergePathDirs(...groups: Array): string { + const seen = new Set(); + const out: string[] = []; + for (const group of groups) { + if (group === undefined) continue; + const dirs = Array.isArray(group) ? group : group.split(path.delimiter); + for (const dir of dirs) { + const trimmed = dir.trim(); + if (!trimmed || seen.has(trimmed)) continue; + seen.add(trimmed); + out.push(trimmed); + } + } + return out.join(path.delimiter); +} + +export function findExecutable( + name: string, + pathEnv: string, + isExecutable: (filePath: string) => boolean = isExecutableFile, +): string | undefined { + for (const dir of pathEnv.split(path.delimiter)) { + const trimmed = dir.trim(); + if (!trimmed) continue; + const candidate = path.join(trimmed, name); + if (isExecutable(candidate)) return candidate; + } + return undefined; +} + +export function parseLoginShellPath(stdout: string, marker = LOGIN_SHELL_PATH_MARKER): string | undefined { + const idx = stdout.lastIndexOf(marker); + if (idx === -1) return undefined; + const rest = stdout.slice(idx + marker.length); + const end = rest.search(/[\r\n]/); + const value = (end === -1 ? rest : rest.slice(0, end)).trim(); + return value.length > 0 ? value : undefined; +} + +export function readLoginShellPath(): string | undefined { + if (process.platform === "win32") return undefined; + const shell = process.env["SHELL"] || (process.platform === "darwin" ? "/bin/zsh" : "/bin/bash"); + try { + const result = spawnSync(shell, ["-lc", `printf '%s' '${LOGIN_SHELL_PATH_MARKER}'"$PATH"`], { + encoding: "utf8", + timeout: LOGIN_SHELL_TIMEOUT_MS, + env: { ...process.env, TERM: "dumb", SHELL: shell }, + stdio: ["ignore", "pipe", "pipe"], + }); + if (!result.stdout) return undefined; + return parseLoginShellPath(result.stdout); + } catch { + return undefined; + } +} + +function orfsNearOpenroad( + openroadBin: string, + dirExistsFn: (dirPath: string) => boolean, +): string | undefined { + let dir = path.dirname(openroadBin); + for (let i = 0; i < 8; i++) { + const flow = path.join(dir, "flow"); + if (dirExistsFn(path.join(flow, "designs")) && dirExistsFn(path.join(flow, "platforms"))) { + return flow; + } + const parent = path.dirname(dir); + if (parent === dir) break; + dir = parent; + } + return undefined; +} + +function looksLikeOrfsFlow(flowPath: string, dirExistsFn: (dirPath: string) => boolean): boolean { + return dirExistsFn(path.join(flowPath, "designs")) && dirExistsFn(path.join(flowPath, "platforms")); +} + +export function defaultPathEnvDeps(env: NodeJS.ProcessEnv = process.env): PathEnvDeps { + const homedir = (): string => os.homedir(); + const platform = process.platform; + return { + env, + isExecutable: isExecutableFile, + dirExists, + extraBinDirs: () => defaultExtraBinDirs(homedir(), platform), + extraOrfsDirs: () => defaultOrfsFlowCandidates(homedir()), + readLoginShellPath, + }; +} + +export function enrichInheritedEnv(deps: PathEnvDeps): { + path: string; + openroad: string | undefined; + orfsFlowPath: string | undefined; +} { + const currentPath = deps.env["PATH"] ?? ""; + let openroad = findExecutable("openroad", currentPath, deps.isExecutable); + let nextPath = currentPath; + + if (!openroad) { + nextPath = mergePathDirs(deps.readLoginShellPath(), currentPath, deps.extraBinDirs()); + openroad = findExecutable("openroad", nextPath, deps.isExecutable); + if (openroad) { + nextPath = mergePathDirs([path.dirname(openroad)], nextPath); + } + deps.env["PATH"] = nextPath; + } + + const existingOrfs = deps.env["ORFS_FLOW_PATH"]?.trim(); + let orfsFlowPath = existingOrfs && existingOrfs.length > 0 ? existingOrfs : undefined; + if (!orfsFlowPath) { + const near = openroad ? orfsNearOpenroad(openroad, deps.dirExists) : undefined; + const found = near ?? deps.extraOrfsDirs().find((p) => looksLikeOrfsFlow(p, deps.dirExists)); + if (found) { + orfsFlowPath = found; + deps.env["ORFS_FLOW_PATH"] = found; + } + } + + return { path: deps.env["PATH"] ?? nextPath, openroad, orfsFlowPath }; +} + +export function applyInheritedEnv(env: NodeJS.ProcessEnv = process.env): void { + enrichInheritedEnv(defaultPathEnvDeps(env)); +} diff --git a/typescript/src/constants.ts b/typescript/src/constants.ts index 92d6fc1..7d3503b 100644 --- a/typescript/src/constants.ts +++ b/typescript/src/constants.ts @@ -25,3 +25,36 @@ export const SLOW_OPERATION_THRESHOLD = 1.0; // Bounds memory on long-lived sessions; oldest entries are dropped when // exceeded. export const MAX_COMMAND_HISTORY = 1000; + +/** + * PTY geometry and terminal type. + * + * OpenROAD runs a readline-style line editor when stdin is a TTY. At a narrow + * width that editor horizontally scrolls long commands, redrawing the whole + * visible window once per character -- a single 75-character read_liberty turns + * into a cascade of near-identical lines that swamps the real output. A width + * far beyond any realistic command avoids the redraw entirely. + * + * TERM=dumb tells readline to skip the fancy editing layer (bracketed paste, + * horizontal scroll, cursor addressing) that we neither need nor can interpret. + */ +export const PTY_COLS = 4096; +export const PTY_ROWS = 24; +export const PTY_TERM = "dumb"; + +/** + * Line terminator written after each command. A real Enter key sends carriage + * return, and a line editor in raw mode binds CR -- not LF -- to accept-line. + * Sending LF can leave the command sitting unsubmitted in the edit buffer. + */ +export const PTY_LINE_TERMINATOR = "\r"; + +/** + * Budget for the startup handshake. This has to cover the whole cold start -- + * loading OpenROAD and reaching its prompt -- not just the probe round-trip, + * because spawning does not wait for readiness. OpenROAD is native C++ and a + * healthy start is sub-second locally, so this is a few seconds of margin for + * a slow disk/container, not an expected duration: the handshake exists to + * catch a session that will never respond, not to tolerate a slow one. + */ +export const SESSION_HANDSHAKE_TIMEOUT_MS = 10_000; diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index 138fb17..b14a657 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -1,5 +1,6 @@ import { Mutex } from "async-mutex"; import { randomUUID } from "node:crypto"; +import { SESSION_HANDSHAKE_TIMEOUT_MS } from "../constants.js"; import { getSettings } from "../config/settings.js"; import type { Settings } from "../config/settings.js"; import { getLogger } from "../utils/logging.js"; @@ -39,6 +40,11 @@ export class OpenROADManager { private readonly maxSessions: number; private readonly defaultTimeoutMs: number; private readonly defaultBufferSize: number; + // Covers the whole cold start, not just the probe round-trip: start() only + // spawns, so OpenROAD still has to load and reach its prompt inside this + // budget. See SESSION_HANDSHAKE_TIMEOUT_MS for why this is a few seconds of + // margin, not a generous allowance. + private readonly handshakeTimeoutMs: number = SESSION_HANDSHAKE_TIMEOUT_MS; constructor(maxSessions?: number) { this.maxSessions = maxSessions ?? this.settings.MAX_SESSIONS; @@ -50,14 +56,20 @@ export class OpenROADManager { async createSession(opts: CreateSessionOptions = {}): Promise { const sessionId = opts.sessionId ?? randomUUID().slice(0, 8); - return this.cleanupLock.runExclusive(async () => { + // Reserve the id under the lock, then release it: spawning OpenROAD and + // waiting out its handshake takes seconds, and holding the manager-wide + // lock for that long stalls every other create/terminate/list. + await this.cleanupLock.runExclusive(async () => { await this._cleanupTerminatedSessions(); if (this.sessions.has(sessionId)) { throw new SessionError(`Session ${sessionId} already exists`, sessionId); } - const activeCount = this._countActive(); + // Reservations count towards the cap: startup now happens outside the + // lock, so without this concurrent creates would all see the same + // pre-spawn total and admit past maxSessions together. + const activeCount = this._countActive() + this._countReserved(); if (activeCount >= this.maxSessions) { throw new SessionError( `Maximum session limit reached (${this.maxSessions}). Currently ${activeCount} active sessions.`, @@ -67,23 +79,36 @@ export class OpenROADManager { // Placeholder distinguishes "creating" (null) from "not found" (absent). this.sessions.set(sessionId, null); + }); + try { + // 0 (and undefined) fall back to the default so a zero-capacity buffer + // can't silently drop all output. + const bufferSize = opts.bufferSize && opts.bufferSize > 0 ? opts.bufferSize : this.defaultBufferSize; + const session = new InteractiveSession(sessionId, bufferSize); try { - // 0 (and undefined) fall back to the default so a zero-capacity buffer - // can't silently drop all output. - const bufferSize = opts.bufferSize && opts.bufferSize > 0 ? opts.bufferSize : this.defaultBufferSize; - const session = new InteractiveSession(sessionId, bufferSize); await session.start(opts.command, opts.env, opts.cwd); + // Never hand out a session that cannot run commands. + await session.verifyResponsive(this.handshakeTimeoutMs); + } catch (e) { + // start() may have spawned a process before the failure; without this + // an unresponsive session leaves an orphaned OpenROAD behind. + await session.terminate(true).catch(() => { /* best effort */ }); + throw e; + } + await this.cleanupLock.runExclusive(() => { this.sessions.set(sessionId, session); - this.logger.info(`Created session ${sessionId}, total sessions: ${this.sessions.size}`); - return sessionId; - } catch (e) { + }); + this.logger.info(`Created session ${sessionId}, total sessions: ${this.sessions.size}`); + return sessionId; + } catch (e) { + await this.cleanupLock.runExclusive(() => { this.sessions.delete(sessionId); - this.logger.error(`Failed to create session ${sessionId}: ${String(e)}`); - throw new SessionError(`Failed to create session: ${String(e)}`, sessionId); - } - }); + }); + this.logger.error(`Failed to create session ${sessionId}: ${String(e)}`); + throw new SessionError(`Failed to create session: ${String(e)}`, sessionId); + } } async executeCommand(sessionId: string, command: string, timeoutMs?: number): Promise { @@ -92,8 +117,7 @@ export class OpenROADManager { // instant timeout. const actualTimeout = timeoutMs && timeoutMs > 0 ? timeoutMs : this.defaultTimeoutMs; - await session.sendCommand(command); - return session.readOutput(actualTimeout); + return session.runCommand(command, actualTimeout); } async getSessionInfo(sessionId: string): Promise { @@ -246,6 +270,15 @@ export class OpenROADManager { return count; } + /** Ids claimed by an in-flight createSession that has not spawned yet. */ + private _countReserved(): number { + let count = 0; + for (const session of this.sessions.values()) { + if (session === null) count++; + } + return count; + } + private _initializedSessions(): Array<[string, InteractiveSession]> { const result: Array<[string, InteractiveSession]> = []; for (const [sid, session] of this.sessions) { diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index ecd93b6..f76deec 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -3,8 +3,17 @@ import { spawn } from "node-pty"; import type { IPty, IDisposable } from "node-pty"; import { getSettings } from "../config/settings.js"; import type { Settings } from "../config/settings.js"; +import { PTY_COLS, PTY_ROWS, PTY_TERM } from "../constants.js"; import { PTYError } from "./models.js"; +/** Terminal geometry must be a positive whole number of cells. Anything else -- + * blank, non-numeric, zero or negative -- is not a usable size, so fall back + * rather than hand node-pty a dimension it cannot honour. */ +function positiveIntOr(value: string | undefined, fallback: number): number { + const n = Number(value); + return Number.isInteger(n) && n > 0 ? n : fallback; +} + export class PtyHandler { private _ptyProcess: IPty | null = null; private _alive = false; @@ -68,20 +77,22 @@ export class PtyHandler { try { this.validateCommand(command); + // Terminal settings come first so an explicit caller env can override + // them; previously they were applied last and silently won. const processEnv: Record = { ...Object.fromEntries( Object.entries(process.env).filter((e): e is [string, string] => e[1] !== undefined), ), + TERM: PTY_TERM, + COLUMNS: String(PTY_COLS), + LINES: String(PTY_ROWS), ...env, - TERM: "xterm-256color", - COLUMNS: "80", - LINES: "24", }; this._ptyProcess = spawn(command[0]!, command.slice(1), { - name: "xterm-256color", - cols: 80, - rows: 24, + name: processEnv.TERM ?? PTY_TERM, + cols: positiveIntOr(processEnv.COLUMNS, PTY_COLS), + rows: positiveIntOr(processEnv.LINES, PTY_ROWS), cwd: cwd ?? process.cwd(), env: processEnv, }); diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 92646d8..84ad0c2 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -15,12 +15,44 @@ import { BYTES_TO_MB, MAX_COMMAND_COMPLETION_WINDOW, MAX_COMMAND_HISTORY, + PTY_LINE_TERMINATOR, UTILIZATION_PERCENTAGE_BASE, } from "../constants.js"; import { CircularBuffer } from "./buffer.js"; import { SessionError, SessionTerminatedError } from "./models.js"; import { PtyHandler } from "./pty_handler.js"; +/** + * Completion sentinel. After each user command we emit a marker line and read + * until it appears, which is the only reliable signal that OpenROAD finished: + * output alone cannot distinguish "still working" from "done", and a quiet + * period is not a completion signal for commands that think silently for + * minutes (read_db, repair_timing, detailed route). + * + * The Tcl is written so the assembled marker never appears in the PTY's echo of + * the input line -- `join` builds "ORMCP-DONE-" at runtime, so a search + * for that literal can only match real stdout, never the echoed source. + */ +const SENTINEL_MARKER = (nonce: string): string => `ORMCP-DONE-${nonce}`; +const SENTINEL_COMMAND = (nonce: string): string => `puts "[join {ORMCP DONE ${nonce}} -]"`; + +/** Matches any sentinel, including a stale one left by a timed-out command. */ +const ANY_SENTINEL_LINE = /ORMCP[-\s]DONE[-\s][0-9a-z]+/i; + +/** + * Startup probe. Uses the same runtime-assembly trick as the sentinel for the + * same reason: a probe whose token appears verbatim in its own source would be + * satisfied by the PTY's echo, so a session that echoes every keystroke but + * executes nothing -- precisely the failure this guards against -- would pass. + */ +const PROBE_COMMAND = `puts "[join {ORMCP READY OK} -]"`; +const PROBE_TOKEN = "ORMCP-READY-OK"; + +/** Liveness re-check cadence while blocked waiting for the sentinel. Bounded so + * a process that dies without emitting output is noticed promptly rather than + * at the full command timeout. */ +const SENTINEL_POLL_MS = 200; + const ERROR_PATTERNS: Array<[RegExp, string]> = [ [/invalid command name "([^"]+)"/i, "Invalid command: {0}"], [/wrong # args: should be "([^"]+)"/i, "Wrong arguments for command: {0}"], @@ -35,6 +67,11 @@ const ERROR_PATTERNS: Array<[RegExp, string]> = [ [/Error: net ([^\s]+) not found/i, "Net not found: {0}"], [/Error: clock ([^\s]+) not found/i, "Clock not found: {0}"], [/Error: no clocks defined/i, "No clocks defined"], + // OpenROAD's native log format: [ERROR RSZ-0089] message. Must precede the + // generic "Error:" rules — those look for a colon, so this form was being + // returned as success with the failure sitting in the output text. + [/\[ERROR\s+([A-Z]+-\d+)\]\s*([^\r\n]+)/, "OpenROAD {0}: {1}"], + [/\[FATAL\s+([A-Z]+-\d+)\]\s*([^\r\n]+)/, "OpenROAD fatal {0}: {1}"], [/Error: (.+?)(?:\r?\n|$)/im, "Error: {0}"], [/ERROR: (.+?)(?:\r?\n|$)/m, "Error: {0}"], [/FATAL: (.+?)(?:\r?\n|$)/m, "Fatal error: {0}"], @@ -65,6 +102,7 @@ export class InteractiveSession { private _inputQueue: string[] = []; private _inputWaiters: Array<() => void> = []; private _isShutdown = false; + private _sentinelCounter = 0; private _writerTask: Promise | null = null; // Serialises terminate()/cleanup() so concurrent callers cannot double-kill // the process or deliver a stale exit code to waiters. @@ -200,16 +238,186 @@ export class InteractiveSession { this.commandHistory.shift(); } - const data = command.endsWith("\n") ? command : command + "\n"; - this._inputQueue.push(data); + this._enqueueRaw(command); this.commandCount++; this.totalCommandsExecuted++; this.lastActivity = new Date(); + } + /** Queue a line for the writer task without touching history or counters. + * Used for the internal completion sentinel, which is bookkeeping rather + * than a user command and must not appear in the audit trail. */ + private _enqueueRaw(command: string): void { + // Every embedded newline is a line the editor has to accept, not just the + // final one, so normalise all of them rather than only the terminator. + const line = command.replace(/\r?\n/g, PTY_LINE_TERMINATOR).replace(/\r+$/, ""); + this._inputQueue.push(line + PTY_LINE_TERMINATOR); const waiters = this._inputWaiters.splice(0); for (const w of waiters) w(); } + /** + * Prove the session can actually execute a command before it is handed out. + * + * A PTY-attached OpenROAD can come up healthy-looking -- process alive, + * prompt printed -- while its line editor never accepts the lines we write, + * so every command silently does nothing. Failing here turns that into an + * immediate, obvious session-creation error instead of a run's worth of + * results that were never computed. + */ + async verifyResponsive(timeoutMs = 15000): Promise { + const result = await this.runCommand(PROBE_COMMAND, timeoutMs); + if (!result.output.includes(PROBE_TOKEN)) { + throw new SessionError( + `Session ${this.sessionId} started but is not executing commands: the OpenROAD ` + + `process is alive yet never ran a probe command within ${timeoutMs}ms. This usually ` + + `means its interactive line editor is not accepting input. ` + + `Probe error: ${result.error ?? "none"}. ` + + `Probe returned: ${JSON.stringify(result.output.slice(-200))}`, + this.sessionId, + ); + } + // The probe is plumbing, not user activity; keep it out of the audit trail. + this.commandHistory.length = 0; + this.commandCount = 0; + this.totalCommandsExecuted = 0; + } + + /** + * Send a command and read until it actually completes. + * + * Prefer this over sendCommand + readOutput: readOutput cannot tell a + * finished command from one that has merely gone quiet, so it returns the + * PTY's echo as a successful result for anything slower than a few + * milliseconds. This waits for an explicit sentinel and reports a real error + * when the command does not finish inside timeoutMs. + */ + async runCommand(command: string, timeoutMs = 30000): Promise { + const startTime = Date.now(); + + if (!this.checkAlive()) { + return this._drainTerminatedSession(startTime); + } + + const nonce = `${Date.now().toString(36)}${(this._sentinelCounter++).toString(36)}${Math.floor( + Math.random() * 0xffffff, + ).toString(36)}`; + const marker = SENTINEL_MARKER(nonce); + + await this.sendCommand(command); + this._enqueueRaw(SENTINEL_COMMAND(nonce)); + + const { text, timedOut, died } = await this._readUntilMarker(marker, startTime, timeoutMs); + + const executionTime = (Date.now() - startTime) / 1000; + const output = ANSIDecoder.cleanOpenroadOutput(this._stripSentinelLines(text)); + + await this._updatePerformanceMetrics(); + this._recordReadResult(output.length, executionTime); + + // An incomplete result outranks anything matched inside it: a command that + // printed "Error:" and then kept running must not look like it finished. + let error: string | null; + if (timedOut) { + error = `CommandTimeout: command did not complete within ${timeoutMs}ms`; + } else if (died) { + error = "SessionTerminated: process exited before the command completed"; + } else { + error = this._detectErrors(output) ?? null; + } + + return { + output, + sessionId: this.sessionId, + timestamp: new Date().toISOString(), + executionTime, + commandCount: this.commandCount, + bufferSize: this.outputBuffer.size, + error, + }; + } + + /** + * Accumulate output until the sentinel arrives, the deadline passes, or the + * process dies. Accumulation is capped at the buffer's configured size, + * keeping the tail, so a verbose multi-megabyte command cannot grow the heap + * without bound. The sentinel is last, so trimming the front never loses it. + */ + private async _readUntilMarker( + marker: string, + startTime: number, + timeoutMs: number, + ): Promise<{ text: string; timedOut: boolean; died: boolean }> { + const cap = Math.max(this.outputBuffer.maxSize, 1); + let text = ""; + + const absorb = (chunks: string[]): void => { + if (chunks.length === 0) return; + text += chunks.join(""); + if (text.length > cap) text = text.slice(text.length - cap); + }; + + for (;;) { + absorb(await this.outputBuffer.drainAll()); + if (text.includes(marker)) return { text, timedOut: false, died: false }; + + if (!this.checkAlive()) { + absorb(await this.outputBuffer.drainAll()); + return { text, timedOut: false, died: !text.includes(marker) }; + } + + const remaining = timeoutMs - (Date.now() - startTime); + if (remaining <= 0) return { text, timedOut: true, died: false }; + + // Capped so process death (which appends nothing) is still noticed. + await this.outputBuffer.waitForData(Math.min(remaining, SENTINEL_POLL_MS)); + } + } + + /** Remove sentinel echo and marker lines so callers never see the plumbing. + * Matches any sentinel, not just the current one, so a marker left behind by + * a previously timed-out command cannot leak into a later result. */ + private _stripSentinelLines(text: string): string { + if (!ANY_SENTINEL_LINE.test(text)) return text; + // Terminals delimit with CR, LF or CRLF, and a line editor emits bare CRs + // when it redraws. Splitting on LF alone would leave the sentinel sharing a + // segment with real output and discard both. + return text + .split(/\r\n|[\r\n]/) + .filter((line) => !ANY_SENTINEL_LINE.test(line)) + .join("\n"); + } + + /** Shared drain-before-reject path for a session that is already dead. */ + private async _drainTerminatedSession(startTime: number): Promise { + this._signalShutdown(); + const chunks = await this.outputBuffer.drainAll(); + if (chunks.length === 0) { + throw new SessionTerminatedError(`Session ${this.sessionId} is not active`, this.sessionId); + } + const output = ANSIDecoder.cleanOpenroadOutput(this._stripSentinelLines(chunks.join(""))); + const executionTime = (Date.now() - startTime) / 1000; + this._recordReadResult(output.length, executionTime); + return { + output, + sessionId: this.sessionId, + timestamp: new Date().toISOString(), + executionTime, + commandCount: this.commandCount, + bufferSize: this.outputBuffer.size, + error: this._detectErrors(output) ?? null, + }; + } + + /** + * Drain whatever output has arrived, using a quiet period as a stop signal. + * + * This cannot detect command completion -- a command that thinks silently + * for longer than MAX_COMMAND_COMPLETION_WINDOW returns here as an empty + * success. Use runCommand() for anything that needs to know the command + * actually finished; this remains for callers that just want to sample the + * buffer. + */ async readOutput(timeoutMs = 1000): Promise { const startTime = Date.now(); @@ -222,24 +430,7 @@ export class InteractiveSession { // Return whatever is buffered rather than discarding it. // Also signal shutdown here so the writer task is guaranteed to stop // even when readOutput() is the first caller to observe the dead state. - this._signalShutdown(); - const chunks = await this.outputBuffer.drainAll(); - if (chunks.length === 0) { - throw new SessionTerminatedError(`Session ${this.sessionId} is not active`, this.sessionId); - } - const rawOutput = chunks.join(""); - const output = ANSIDecoder.cleanOpenroadOutput(rawOutput); - const executionTime = (Date.now() - startTime) / 1000; - this._recordReadResult(output.length, executionTime); - return { - output, - sessionId: this.sessionId, - timestamp: new Date().toISOString(), - executionTime, - commandCount: this.commandCount, - bufferSize: this.outputBuffer.size, - error: this._detectErrors(output) ?? null, - }; + return this._drainTerminatedSession(startTime); } const collected: string[] = []; @@ -406,10 +597,11 @@ export class InteractiveSession { for (const [pattern, template] of ERROR_PATTERNS) { const match = pattern.exec(output); if (match) { - const capture = match[1]; // Function replacement so `$&`/`$1`/`$$` inside the captured error text // are inserted literally, not reinterpreted as replacement patterns. - return capture ? template.replace("{0}", () => capture.trim()) : template; + return template + .replace("{0}", () => (match[1] ?? "").trim()) + .replace("{1}", () => (match[2] ?? "").trim()); } } diff --git a/typescript/src/main.ts b/typescript/src/main.ts index b8cc382..7c6f02a 100644 --- a/typescript/src/main.ts +++ b/typescript/src/main.ts @@ -1,12 +1,14 @@ #!/usr/bin/env node import { parseCliArgs } from "./config/cli.js"; +import { applyInheritedEnv } from "./config/path_env.js"; import { initSettings } from "./config/settings.js"; import { EXIT_CODE_ERROR } from "./constants.js"; import { ValidationError } from "./exceptions.js"; /** - * Entry point. The eager work (settings validation, CLI parsing) runs before - * any module that reads settings or builds a logger is imported. + * Entry point. The eager work (PATH inheritance, settings validation, CLI + * parsing) runs before any module that reads settings or builds a logger is + * imported. * * This ordering matters and is why `logging` and `server` are dynamic imports: * `utils/logging` calls `getSettings()` at module load to seed the root logger, @@ -17,6 +19,7 @@ import { ValidationError } from "./exceptions.js"; */ async function main(): Promise { try { + applyInheritedEnv(); initSettings(); } catch (e) { throw new ValidationError(e instanceof Error ? e.message : String(e)); diff --git a/typescript/src/tools/report_images.ts b/typescript/src/tools/report_images.ts index 9e0f846..cd9fd56 100644 --- a/typescript/src/tools/report_images.ts +++ b/typescript/src/tools/report_images.ts @@ -41,7 +41,7 @@ const IMAGE_TYPE_MAPPING: Record = { * ["unknown", "unknown"] for files with no underscore or unrecognised keys. */ export function classifyImageType(filename: string): [string, string] { - const basename = path.basename(filename, path.extname(filename)); + const basename = stripImageExtension(path.basename(filename)); const underscoreIdx = basename.indexOf("_"); let stage: string; let key: string; @@ -97,14 +97,50 @@ function availableRuns(reportsBase: string): string[] { } } -function findWebpFiles(dir: string): string[] { +/** + * Report image extensions. + * + * ORFS does not consistently name these: depending on the build, save_images + * writes `final_all.webp` or `final_all.webp.png` (a PNG carrying a doubled + * extension). Matching only `.webp` silently finds nothing on the latter, so + * accept both and let the decoder sort out the actual format. + */ +const IMAGE_EXTENSIONS = [".webp.png", ".webp", ".png"]; + +export function isReportImage(filename: string): boolean { + return IMAGE_EXTENSIONS.some((ext) => filename.toLowerCase().endsWith(ext)); +} + +/** + * Format of the bytes actually returned, which is not always WebP. + * + * Only the compression path re-encodes; images small enough to send as-is are + * returned byte-for-byte from disk. Since `.webp.png` files really are PNG, + * reporting a blanket "webp" would misdescribe the payload to any consumer that + * trusts this field instead of sniffing the header. + */ +function imageFormat(filename: string, compressionApplied: boolean): string { + if (compressionApplied) return "webp"; + return filename.toLowerCase().endsWith(".webp") ? "webp" : "png"; +} + +/** Strip the report-image extension, including the doubled `.webp.png` form. */ +function stripImageExtension(filename: string): string { + const lower = filename.toLowerCase(); + for (const ext of IMAGE_EXTENSIONS) { + if (lower.endsWith(ext)) return filename.slice(0, filename.length - ext.length); + } + return filename; +} + +function findImageFiles(dir: string): string[] { const results: string[] = []; for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { if (entry.isSymbolicLink()) continue; const full = path.join(dir, entry.name); if (entry.isDirectory()) { - results.push(...findWebpFiles(full)); - } else if (entry.isFile() && entry.name.endsWith(".webp")) { + results.push(...findImageFiles(full)); + } else if (entry.isFile() && isReportImage(entry.name)) { results.push(full); } } @@ -250,17 +286,34 @@ export class ListReportImagesTool extends BaseTool { try { let files: string[]; try { - files = findWebpFiles(runPath); + files = findImageFiles(runPath); } catch { files = []; } if (files.length === 0) { + // An empty result is ambiguous: no images generated, or images present + // under a name we do not recognise. Say which, so a caller is never + // left guessing whether the run or the tool is at fault. + let hint = "No report images found in this run directory."; + try { + const others = fs + .readdirSync(runPath, { withFileTypes: true }) + .filter((e) => e.isFile() && /\.(png|jpe?g|gif|svg|webp)$/i.test(e.name)) + .map((e) => e.name); + if (others.length > 0) { + hint = + `Found ${others.length} image-like file(s) that do not match the expected ` + + `extensions (${IMAGE_EXTENSIONS.join(", ")}): ${others.slice(0, 10).join(", ")}`; + } + } catch { /* directory listing is best effort */ } + return this.formatResult( ListImagesResult.parse({ runPath, totalImages: 0, imagesByStage: {}, + message: hint, }) as unknown as Record, ); } @@ -357,11 +410,11 @@ export class ReadReportImageTool extends BaseTool { ); } - if (!imageName.endsWith(".webp")) { + if (!isReportImage(imageName)) { return this.formatResult( ReadImageResult.parse({ error: "InvalidImageName", - message: `Image '${imageName}' must have a .webp extension`, + message: `Image '${imageName}' must end in one of: ${IMAGE_EXTENSIONS.join(", ")}`, }) as unknown as Record, ); } @@ -392,7 +445,7 @@ export class ReadReportImageTool extends BaseTool { if (!fs.existsSync(imagePath)) { let available: string[] = []; try { - available = findWebpFiles(runPath).map((f) => path.basename(f)); + available = findImageFiles(runPath).map((f: string) => path.basename(f)); } catch { available = []; } @@ -434,7 +487,7 @@ export class ReadReportImageTool extends BaseTool { const metadata = ImageMetadata.parse({ filename: imageName, - format: "webp", + format: imageFormat(imageName, r.compressionApplied), sizeBytes: r.compressedSize, width: r.width, height: r.height,