From 333950047b599429e8cae420920fb0adbbf53de6 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Tue, 16 Jun 2026 21:22:32 -0600 Subject: [PATCH 01/44] add base tool class with camelcase to snake case serialization boundary --- typescript/src/tools/base.ts | 39 ++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 typescript/src/tools/base.ts diff --git a/typescript/src/tools/base.ts b/typescript/src/tools/base.ts new file mode 100644 index 0000000..7b34706 --- /dev/null +++ b/typescript/src/tools/base.ts @@ -0,0 +1,39 @@ +import type { OpenROADManager } from "../core/manager.js"; + +function camelToSnakeKey(key: string): string { + return key.replace(/[A-Z]/g, (ch) => `_${ch.toLowerCase()}`); +} + +/** + * Recursively converts camelCase object keys to snake_case. + * Idempotent on already-snake_case strings (no uppercase → no change), + * so opaque snake_case payloads (SessionDetailedMetrics, ManagerMetrics, + * CommandHistoryEntry) pass through unchanged. + */ +export function toSnakeCase(value: unknown): unknown { + if (Array.isArray(value)) return value.map(toSnakeCase); + if (value !== null && typeof value === "object") { + return Object.fromEntries( + Object.entries(value as Record).map(([k, v]) => [ + camelToSnakeKey(k), + toSnakeCase(v), + ]), + ); + } + return value; +} + +/** + * Base class for all MCP tool implementations. + * + * Provides the manager dependency and a serialization helper that converts the + * camelCase domain model to the snake_case MCP wire format in one place. + * Each subclass declares its own typed execute() signature. + */ +export abstract class BaseTool { + protected constructor(protected readonly manager: OpenROADManager) {} + + protected formatResult(result: Record): string { + return JSON.stringify(toSnakeCase(result)); + } +} From 417e3d275ac2eedc9adb4de47c0bf93bdd3c0cfe Mon Sep 17 00:00:00 2001 From: kartikloops Date: Tue, 16 Jun 2026 21:22:37 -0600 Subject: [PATCH 02/44] add interactive tool classes porting session management and command execution from python --- typescript/src/tools/interactive.ts | 442 ++++++++++++++++++++++++++++ 1 file changed, 442 insertions(+) create mode 100644 typescript/src/tools/interactive.ts diff --git a/typescript/src/tools/interactive.ts b/typescript/src/tools/interactive.ts new file mode 100644 index 0000000..8433ced --- /dev/null +++ b/typescript/src/tools/interactive.ts @@ -0,0 +1,442 @@ +import { getSettings } from "../config/settings.js"; +import { + isExecCommand, + isQueryCommand, +} from "../config/command_whitelist.js"; +import type { OpenROADManager } from "../core/manager.js"; +import { + InteractiveSessionListResult, + SessionHistoryResult, + SessionInspectionResult, + SessionMetricsResult, + SessionTerminationResult, +} from "../core/models.js"; +import type { + InteractiveExecResult, + InteractiveSessionInfo, +} from "../core/models.js"; +import { + SessionError, + SessionNotFoundError, + SessionTerminatedError, +} from "../interactive/models.js"; +import { getLogger } from "../utils/logging.js"; +import { BaseTool, toSnakeCase } from "./base.js"; + +const logger = getLogger("tools.interactive"); + +// --------------------------------------------------------------------------- +// Module-level helpers +// --------------------------------------------------------------------------- + +/** Emulate Python's repr() for simple strings: single-quoted with escaping. */ +function pyRepr(s: string): string { + const escaped = s.replace(/\\/g, "\\\\").replace(/'/g, "\\'"); + return `'${escaped}'`; +} + +/** Build a blank InteractiveExecResult skeleton for error paths. */ +function blankExecResult( + sessionId: string | null, + error: string, +): InteractiveExecResult { + return { + output: "", + sessionId, + timestamp: new Date().toISOString(), + executionTime: 0.0, + commandCount: 0, + bufferSize: 0, + error, + }; +} + +/** + * Returns an InteractiveExecResult representing a session-not-found condition. + * Error message matches the Python server byte-for-byte. + */ +function sessionNotFoundExecResult( + sessionId: string | null, + error: unknown, +): InteractiveExecResult { + return { + output: `Error: Session '${sessionId}' not found.`, + sessionId, + timestamp: new Date().toISOString(), + executionTime: 0.0, + commandCount: 0, + bufferSize: 0, + error: String(error), + }; +} + +/** + * Build and serialize a blocked-command result. + * Matches Python's _blocked_error() output exactly, including repr() quoting. + */ +function blockedError( + command: string, + blockedVerb: string, + sessionId: string | null, +): string { + const base: InteractiveExecResult = { + output: "", + sessionId, + timestamp: new Date().toISOString(), + executionTime: 0.0, + commandCount: 0, + bufferSize: 0, + error: `CommandBlocked: '${blockedVerb}'`, + }; + const message = `Command blocked: '${blockedVerb}' is not on the OpenROAD allowlist.\nFull command: ${pyRepr(command)}`; + return JSON.stringify(toSnakeCase({ ...base, message })); +} + +/** + * Gate a command through the Tcl whitelist when WHITELIST_ENABLED is set. + * Returns a serialised blocked-error JSON string when the command is rejected, + * or null when it is allowed (or when the whitelist is disabled). + */ +function applyWhitelist( + command: string, + validator: (cmd: string) => [boolean, string | null], + sessionId: string | null, +): string | null { + const settings = getSettings(); + if (!settings.WHITELIST_ENABLED) return null; + const [allowed, blockedVerb] = validator(command); + if (!allowed && blockedVerb !== null) { + logger.warn( + `Command blocked: '${blockedVerb}' for session ${sessionId ?? "new"}`, + ); + return blockedError(command, blockedVerb, sessionId); + } + return null; +} + +// --------------------------------------------------------------------------- +// Tool classes +// --------------------------------------------------------------------------- + +/** Read-only query tool: report_*, get_*, check_*, sta, help, version, etc. */ +export class QueryShellTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute( + command: string, + sessionId?: string | null, + timeoutMs?: number | null, + ): Promise { + const sid = sessionId ?? null; + + const blocked = applyWhitelist(command, isQueryCommand, sid); + if (blocked !== null) return blocked; + + let resolvedId = sid; + try { + if (resolvedId === null || resolvedId === undefined) { + resolvedId = await this.manager.createSession({}); + } + const result = await this.manager.executeCommand( + resolvedId, + command, + timeoutMs ?? undefined, + ); + return this.formatResult(result as unknown as Record); + } catch (e) { + if (e instanceof SessionNotFoundError) { + return this.formatResult( + sessionNotFoundExecResult( + resolvedId, + e, + ) as unknown as Record, + ); + } + if (e instanceof SessionTerminatedError || e instanceof SessionError) { + return this.formatResult( + blankExecResult( + resolvedId, + (e as Error).message, + ) as unknown as Record, + ); + } + return this.formatResult( + blankExecResult( + resolvedId, + `Unexpected error: ${(e as Error).message ?? String(e)}`, + ) as unknown as Record, + ); + } + } +} + +/** State-modifying exec tool: set_*, create_*, read_*, write_*, flow/repair, etc. */ +export class ExecShellTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute( + command: string, + sessionId?: string | null, + timeoutMs?: number | null, + ): Promise { + const sid = sessionId ?? null; + + const blocked = applyWhitelist(command, isExecCommand, sid); + if (blocked !== null) return blocked; + + let resolvedId = sid; + try { + if (resolvedId === null || resolvedId === undefined) { + resolvedId = await this.manager.createSession({}); + } + const result = await this.manager.executeCommand( + resolvedId, + command, + timeoutMs ?? undefined, + ); + return this.formatResult(result as unknown as Record); + } catch (e) { + if (e instanceof SessionNotFoundError) { + return this.formatResult( + sessionNotFoundExecResult( + resolvedId, + e, + ) as unknown as Record, + ); + } + if (e instanceof SessionTerminatedError || e instanceof SessionError) { + return this.formatResult( + blankExecResult( + resolvedId, + (e as Error).message, + ) as unknown as Record, + ); + } + return this.formatResult( + blankExecResult( + resolvedId, + `Unexpected error: ${(e as Error).message ?? String(e)}`, + ) as unknown as Record, + ); + } + } +} + +/** Lists all active and terminated sessions tracked by the manager. */ +export class ListSessionsTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute(): Promise { + try { + const sessions = await this.manager.listSessions(); + const activeCount = sessions.filter((s) => s.isAlive).length; + return this.formatResult( + InteractiveSessionListResult.parse({ + sessions, + totalCount: sessions.length, + activeCount, + }) as unknown as Record, + ); + } catch (e) { + return this.formatResult( + InteractiveSessionListResult.parse({ + error: String(e), + }) as unknown as Record, + ); + } + } +} + +/** Creates a new OpenROAD interactive session. */ +export class CreateSessionTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute( + sessionId?: string, + command?: string[], + env?: Record, + cwd?: string, + ): Promise { + try { + const opts = { + ...(sessionId !== undefined && { sessionId }), + ...(command !== undefined && { command }), + ...(env !== undefined && { env }), + ...(cwd !== undefined && { cwd }), + }; + const id = await this.manager.createSession(opts); + const info = await this.manager.getSessionInfo(id); + return this.formatResult(info as unknown as Record); + } catch (e) { + const errInfo: InteractiveSessionInfo = { + sessionId: sessionId ?? "unknown", + createdAt: new Date().toISOString(), + isAlive: false, + commandCount: 0, + bufferSize: 0, + uptimeSeconds: null, + state: null, + error: String(e), + }; + return this.formatResult(errInfo as unknown as Record); + } + } +} + +/** Terminates an existing session by ID. */ +export class TerminateSessionTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute(sessionId: string, force = false): Promise { + let wasAlive = true; + try { + await this.manager.getSessionInfo(sessionId); + } catch (e) { + if (e instanceof SessionNotFoundError) { + wasAlive = false; + } + } + + try { + await this.manager.terminateSession(sessionId, force); + return this.formatResult( + SessionTerminationResult.parse({ + sessionId, + terminated: true, + wasAlive, + force, + }) as unknown as Record, + ); + } catch (e) { + if (e instanceof SessionNotFoundError) { + return this.formatResult( + SessionTerminationResult.parse({ + sessionId, + terminated: false, + error: String(e), + }) as unknown as Record, + ); + } + return this.formatResult( + SessionTerminationResult.parse({ + sessionId, + terminated: false, + error: `Termination failed: ${(e as Error).message ?? String(e)}`, + }) as unknown as Record, + ); + } + } +} + +/** Returns detailed metrics for a single session. */ +export class InspectSessionTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute(sessionId: string): Promise { + try { + const metrics = await this.manager.inspectSession(sessionId); + return this.formatResult( + SessionInspectionResult.parse({ + sessionId, + metrics, + }) as unknown as Record, + ); + } catch (e) { + if (e instanceof SessionNotFoundError) { + return this.formatResult( + SessionInspectionResult.parse({ + sessionId, + error: String(e), + }) as unknown as Record, + ); + } + return this.formatResult( + SessionInspectionResult.parse({ + sessionId, + error: `Inspection failed: ${(e as Error).message ?? String(e)}`, + }) as unknown as Record, + ); + } + } +} + +/** Returns the command history for a session, with optional limit and search. */ +export class SessionHistoryTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute( + sessionId: string, + limit?: number, + search?: string, + ): Promise { + try { + const history = await this.manager.getSessionHistory(sessionId, limit, search); + return this.formatResult( + SessionHistoryResult.parse({ + sessionId, + history, + totalCommands: history.length, + limit: limit ?? null, + search: search ?? null, + }) as unknown as Record, + ); + } catch (e) { + if (e instanceof SessionNotFoundError) { + return this.formatResult( + SessionHistoryResult.parse({ + sessionId, + error: String(e), + }) as unknown as Record, + ); + } + return this.formatResult( + SessionHistoryResult.parse({ + sessionId, + error: `History retrieval failed: ${(e as Error).message ?? String(e)}`, + }) as unknown as Record, + ); + } + } +} + +/** Returns aggregate metrics across all sessions managed by the manager. */ +export class SessionMetricsTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute(): Promise { + try { + const metrics = await this.manager.sessionMetrics(); + return this.formatResult( + SessionMetricsResult.parse({ metrics }) as unknown as Record< + string, + unknown + >, + ); + } catch (e) { + return this.formatResult( + SessionMetricsResult.parse({ + error: `Metrics retrieval failed: ${(e as Error).message ?? String(e)}`, + }) as unknown as Record, + ); + } + } +} + +// Backwards-compat alias matching Python's InteractiveShellTool = QueryShellTool +export const InteractiveShellTool = QueryShellTool; From ad3a44a10d204d854e2c5e79b8a5a273ba4a60a4 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Tue, 16 Jun 2026 21:23:06 -0600 Subject: [PATCH 03/44] add report image tools using sharp for webp compression and path traversal validation --- typescript/src/tools/report_images.ts | 501 ++++++++++++++++++++++++++ 1 file changed, 501 insertions(+) create mode 100644 typescript/src/tools/report_images.ts diff --git a/typescript/src/tools/report_images.ts b/typescript/src/tools/report_images.ts new file mode 100644 index 0000000..8daf037 --- /dev/null +++ b/typescript/src/tools/report_images.ts @@ -0,0 +1,501 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import sharp from "sharp"; +import type { OpenROADManager } from "../core/manager.js"; +import { + ImageInfo, + ImageMetadata, + ListImagesResult, + ReadImageResult, +} from "../core/models.js"; +import { ValidationError } from "../exceptions.js"; +import { + validatePathSegment, + validateSafePathContainment, +} from "../utils/path_security.js"; +import { getSettings } from "../config/settings.js"; +import { getLogger } from "../utils/logging.js"; +import { BaseTool } from "./base.js"; + +const logger = getLogger("tools.report_images"); + +// --------------------------------------------------------------------------- +// Constants +// --------------------------------------------------------------------------- + +const MAX_BASE64_SIZE_KB = 15; +const MAX_IMAGE_SIZE_MB = 50; + +// --------------------------------------------------------------------------- +// Image type mapping (exact copy of Python dict) +// --------------------------------------------------------------------------- + +const IMAGE_TYPE_MAPPING: Record = { + cts_clk: "clock_visualization", + cts_clk_layout: "clock_layout", + cts_core_clock: "core_clock_visualization", + cts_core_clock_layout: "core_clock_layout", + final_all: "complete_design", + final_clocks: "clock_routing", + final_congestion: "congestion_heatmap", + final_ir_drop: "ir_drop_analysis", + final_placement: "cell_placement", + final_resizer: "resizer_results", + final_routing: "routing_visualization", +}; + +// --------------------------------------------------------------------------- +// Helper functions +// --------------------------------------------------------------------------- + +/** + * Derive the image stage and semantic type from a filename. + * Returns ["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 underscoreIdx = basename.indexOf("_"); + let stage: string; + let key: string; + if (underscoreIdx === -1) { + stage = "unknown"; + key = basename; + } else { + stage = basename.slice(0, underscoreIdx); + key = basename; + } + const type = IMAGE_TYPE_MAPPING[key] ?? "unknown"; + return [stage, type]; +} + +/** + * Verify that `platform` and `design` are known in the current ORFS configuration. + * Throws ValidationError when either is not found. + */ +export function validatePlatformDesign(platform: string, design: string): void { + const settings = getSettings(); + const platforms = settings.platforms; + if (!platforms.includes(platform)) { + throw new ValidationError( + `Platform '${platform}' not found. Available platforms: ${platforms.join(", ") || "none"}`, + ); + } + const designs = settings.designs(platform); + if (!designs.includes(design)) { + throw new ValidationError( + `Design '${design}' not found for platform '${platform}'. Available designs: ${designs.join(", ") || "none"}`, + ); + } +} + +/** + * Resolve and validate the reports base path and per-run sub-directory. + * Returns [reportsBase, runPath] as absolute path strings. + */ +function resolveRunPath( + platform: string, + design: string, + runSlug: string, +): [string, string] { + validatePlatformDesign(platform, design); + validatePathSegment(runSlug, "run_slug"); + const settings = getSettings(); + const reportsBase = path.join(settings.flowPath, "reports", platform, design); + const runPath = path.join(reportsBase, runSlug); + validateSafePathContainment(runPath, reportsBase, "run directory"); + return [reportsBase, runPath]; +} + +/** List available run slugs in reportsBase for error messages. */ +function availableRuns(reportsBase: string): string[] { + try { + return fs + .readdirSync(reportsBase, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => e.name); + } catch { + return []; + } +} + +/** + * Recursively find all .webp files under `dir`, returning absolute paths. + * Requires Node.js ≥ 20 for the `recursive` option on readdirSync. + */ +function findWebpFiles(dir: string): string[] { + const entries = fs.readdirSync(dir, { recursive: true, withFileTypes: true }); + return entries + .filter((e) => e.isFile() && e.name.endsWith(".webp")) + .map((e) => { + // `parentPath` is available in Node 20.12+; `path` is the older alias. + const parent = (e as unknown as { parentPath?: string; path?: string }) + .parentPath ?? (e as unknown as { path: string }).path; + return path.join(parent, e.name); + }); +} + +interface CompressResult { + imageBytes: Buffer; + compressionApplied: boolean; + originalSize: number; + compressedSize: number; + originalWidth: number | null; + originalHeight: number | null; + width: number | null; + height: number | null; +} + +/** + * Load an image and compress it to fit within `maxSizeKb` of base64 output. + * Uses sharp for resizing (lanczos3) and WebP encoding (quality=85). + * Falls back to returning raw bytes when sharp fails, with null dimensions. + */ +async function loadAndCompressImage( + imagePath: string, + maxSizeKb: number = MAX_BASE64_SIZE_KB, +): Promise { + const originalSize = fs.statSync(imagePath).size; + const estimatedBase64 = Math.floor((originalSize * 4) / 3); + + if (estimatedBase64 / 1024 <= maxSizeKb) { + try { + const rawBytes = fs.readFileSync(imagePath); + const meta = await sharp(imagePath).metadata(); + return { + imageBytes: rawBytes, + compressionApplied: false, + originalSize, + compressedSize: originalSize, + originalWidth: meta.width ?? null, + originalHeight: meta.height ?? null, + width: meta.width ?? null, + height: meta.height ?? null, + }; + } catch (e) { + logger.warn({ err: e, imagePath }, "sharp.metadata() failed on small image; returning raw bytes with null dims"); + return { + imageBytes: fs.readFileSync(imagePath), + compressionApplied: false, + originalSize, + compressedSize: originalSize, + originalWidth: null, + originalHeight: null, + width: null, + height: null, + }; + } + } + + try { + const targetBytes = Math.floor((maxSizeKb * 1024 * 3) / 4); + const scale = Math.sqrt(targetBytes / originalSize); + const meta = await sharp(imagePath).metadata(); + const origW = meta.width ?? 0; + const origH = meta.height ?? 0; + const newW = Math.max(Math.round(origW * scale), 256); + const newH = Math.max(Math.round(origH * scale), 256); + const compressed = await sharp(imagePath) + .resize(newW, newH, { kernel: "lanczos3" }) + .webp({ quality: 85 }) + .toBuffer(); + return { + imageBytes: compressed, + compressionApplied: true, + originalSize, + compressedSize: compressed.length, + originalWidth: meta.width ?? null, + originalHeight: meta.height ?? null, + width: newW, + height: newH, + }; + } catch (e) { + logger.warn({ err: e, imagePath }, "Image compression failed; returning raw bytes with null dims"); + return { + imageBytes: fs.readFileSync(imagePath), + compressionApplied: false, + originalSize, + compressedSize: originalSize, + originalWidth: null, + originalHeight: null, + width: null, + height: null, + }; + } +} + +// --------------------------------------------------------------------------- +// Tool classes +// --------------------------------------------------------------------------- + +/** Lists .webp report images for a specific platform/design/run. */ +export class ListReportImagesTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute( + platform: string, + design: string, + runSlug: string, + stage = "all", + ): Promise { + let reportsBase: string; + let runPath: string; + + try { + [reportsBase, runPath] = resolveRunPath(platform, design, runSlug); + } catch (e) { + if (e instanceof ValidationError) { + return this.formatResult( + ListImagesResult.parse({ + error: e.constructor.name, + message: e.message, + }) as unknown as Record, + ); + } + return this.formatResult( + ListImagesResult.parse({ + error: "UnexpectedError", + message: (e as Error).message ?? String(e), + }) as unknown as Record, + ); + } + + if (!fs.existsSync(runPath)) { + const runs = availableRuns(reportsBase); + return this.formatResult( + ListImagesResult.parse({ + error: "RunNotFound", + message: `Run directory '${runSlug}' not found. Available runs: ${runs.join(", ") || "none"}`, + }) as unknown as Record, + ); + } + + try { + let files: string[]; + try { + files = findWebpFiles(runPath); + } catch { + files = []; + } + + if (files.length === 0) { + return this.formatResult( + ListImagesResult.parse({ + runPath, + totalImages: 0, + imagesByStage: {}, + }) as unknown as Record, + ); + } + + const imagesByStage: Record = {}; + let total = 0; + + for (const filePath of files) { + const filename = path.basename(filePath); + const [fileStage, type] = classifyImageType(filename); + if (stage !== "all" && stage !== fileStage) continue; + + const stat = fs.statSync(filePath); + const imageInfo = ImageInfo.parse({ + filename, + path: filePath, + sizeBytes: stat.size, + modifiedTime: stat.mtime.toISOString(), + type, + }); + + const bucket = imagesByStage[fileStage] ?? []; + bucket.push(imageInfo); + imagesByStage[fileStage] = bucket; + total++; + } + + // Sort each stage bucket by filename + for (const key of Object.keys(imagesByStage)) { + imagesByStage[key] = (imagesByStage[key] as Array<{ filename: string }>).sort((a, b) => + a.filename.localeCompare(b.filename), + ); + } + + return this.formatResult( + ListImagesResult.parse({ + runPath, + totalImages: total, + imagesByStage, + }) as unknown as Record, + ); + } catch (e) { + return this.formatResult( + ListImagesResult.parse({ + error: "UnexpectedError", + message: (e as Error).message ?? String(e), + }) as unknown as Record, + ); + } + } +} + +/** Reads, optionally compresses, and base64-encodes a single report image. */ +export class ReadReportImageTool extends BaseTool { + constructor(manager: OpenROADManager) { + super(manager); + } + + async execute( + platform: string, + design: string, + runSlug: string, + imageName: string, + ): Promise { + let reportsBase: string; + let runPath: string; + + try { + [reportsBase, runPath] = resolveRunPath(platform, design, runSlug); + } catch (e) { + if (e instanceof ValidationError) { + return this.formatResult( + ReadImageResult.parse({ + error: e.constructor.name, + message: e.message, + }) as unknown as Record, + ); + } + return this.formatResult( + ReadImageResult.parse({ + error: "UnexpectedError", + message: (e as Error).message ?? String(e), + }) as unknown as Record, + ); + } + + try { + validatePathSegment(imageName, "image_name"); + } catch (e) { + return this.formatResult( + ReadImageResult.parse({ + error: (e as ValidationError).constructor.name, + message: (e as Error).message, + }) as unknown as Record, + ); + } + + if (!imageName.endsWith(".webp")) { + return this.formatResult( + ReadImageResult.parse({ + error: "InvalidImageName", + message: `Image '${imageName}' must have a .webp extension`, + }) as unknown as Record, + ); + } + + if (!fs.existsSync(runPath)) { + const runs = availableRuns(reportsBase); + return this.formatResult( + ReadImageResult.parse({ + error: "RunNotFound", + message: `Run directory '${runSlug}' not found. Available runs: ${runs.join(", ") || "none"}`, + }) as unknown as Record, + ); + } + + const imagePath = path.join(runPath, imageName); + + try { + validateSafePathContainment(imagePath, runPath, "image file"); + } catch (e) { + return this.formatResult( + ReadImageResult.parse({ + error: (e as ValidationError).constructor.name, + message: (e as Error).message, + }) as unknown as Record, + ); + } + + if (!fs.existsSync(imagePath)) { + let available: string[] = []; + try { + available = findWebpFiles(runPath).map((f) => path.basename(f)); + } catch { + available = []; + } + return this.formatResult( + ReadImageResult.parse({ + error: "ImageNotFound", + message: `Image '${imageName}' not found. Available images: ${available.join(", ") || "none"}`, + }) as unknown as Record, + ); + } + + const stat = fs.statSync(imagePath); + if (!stat.isFile()) { + return this.formatResult( + ReadImageResult.parse({ + error: "NotAFile", + message: `'${imageName}' is not a regular file`, + }) as unknown as Record, + ); + } + + if (stat.size > MAX_IMAGE_SIZE_MB * 1024 * 1024) { + return this.formatResult( + ReadImageResult.parse({ + error: "FileTooLarge", + message: `Image '${imageName}' exceeds the ${MAX_IMAGE_SIZE_MB} MB size limit`, + }) as unknown as Record, + ); + } + + try { + const r = await loadAndCompressImage(imagePath); + const imageData = r.imageBytes.toString("base64"); + const [stage, type] = classifyImageType(imageName); + const compressionRatio = + r.compressionApplied && r.compressedSize > 0 + ? r.originalSize / r.compressedSize + : null; + + const metadata = ImageMetadata.parse({ + filename: imageName, + format: "webp", + sizeBytes: r.compressedSize, + width: r.width, + height: r.height, + modifiedTime: stat.mtime.toISOString(), + stage, + type, + compressionApplied: r.compressionApplied, + originalSizeBytes: r.compressionApplied ? r.originalSize : null, + originalWidth: r.originalWidth, + originalHeight: r.originalHeight, + compressionRatio, + }); + + return this.formatResult( + ReadImageResult.parse({ + imageData, + metadata, + }) as unknown as Record, + ); + } catch (e) { + if (e instanceof ValidationError) { + return this.formatResult( + ReadImageResult.parse({ + error: e.constructor.name, + message: e.message, + }) as unknown as Record, + ); + } + return this.formatResult( + ReadImageResult.parse({ + error: "UnexpectedError", + message: (e as Error).message ?? String(e), + }) as unknown as Record, + ); + } + } +} + From e517cdb953a53a481ca311eb39b5188fb18261e5 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Tue, 16 Jun 2026 21:23:11 -0600 Subject: [PATCH 04/44] add tools barrel export --- typescript/src/tools/index.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 typescript/src/tools/index.ts diff --git a/typescript/src/tools/index.ts b/typescript/src/tools/index.ts new file mode 100644 index 0000000..bdcee09 --- /dev/null +++ b/typescript/src/tools/index.ts @@ -0,0 +1,13 @@ +export { BaseTool, toSnakeCase } from "./base.js"; +export { + CreateSessionTool, + ExecShellTool, + InspectSessionTool, + InteractiveShellTool, + ListSessionsTool, + QueryShellTool, + SessionHistoryTool, + SessionMetricsTool, + TerminateSessionTool, +} from "./interactive.js"; +export { ListReportImagesTool, ReadReportImageTool, classifyImageType, validatePlatformDesign } from "./report_images.js"; From 9d2bb0dc5cd6c2af5c4160db58af32675234fe2b Mon Sep 17 00:00:00 2001 From: kartikloops Date: Tue, 16 Jun 2026 21:24:44 -0600 Subject: [PATCH 05/44] add tests for interactive shell and session management tools --- .../__tests__/tools/interactive.test.ts | 496 ++++++++++++++++++ 1 file changed, 496 insertions(+) create mode 100644 typescript/__tests__/tools/interactive.test.ts diff --git a/typescript/__tests__/tools/interactive.test.ts b/typescript/__tests__/tools/interactive.test.ts new file mode 100644 index 0000000..bf1ef74 --- /dev/null +++ b/typescript/__tests__/tools/interactive.test.ts @@ -0,0 +1,496 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; +import type { Mock } from "vitest"; +import { QueryShellTool, ExecShellTool, ListSessionsTool, CreateSessionTool, TerminateSessionTool, InspectSessionTool, SessionHistoryTool, SessionMetricsTool, InteractiveShellTool } from "../../src/tools/interactive.js"; +import type { OpenROADManager } from "../../src/core/manager.js"; +import { SessionNotFoundError, SessionTerminatedError, SessionError } from "../../src/interactive/models.js"; +import { SessionState } from "../../src/core/models.js"; +import type { InteractiveExecResult, InteractiveSessionInfo, SessionDetailedMetrics, ManagerMetrics } from "../../src/core/models.js"; + +// --------------------------------------------------------------------------- +// Mock helpers +// --------------------------------------------------------------------------- + +const NOW = "2024-01-01T00:00:00.000Z"; + +function makeExecResult(overrides: Partial = {}): InteractiveExecResult { + return { + output: "test output", + sessionId: "session-1", + timestamp: NOW, + executionTime: 0.1, + commandCount: 1, + bufferSize: 0, + error: null, + ...overrides, + }; +} + +function makeSessionInfo(overrides: Partial = {}): InteractiveSessionInfo { + return { + sessionId: "session-1", + createdAt: NOW, + isAlive: true, + commandCount: 5, + bufferSize: 1024, + uptimeSeconds: 100.0, + state: SessionState.ACTIVE, + error: null, + ...overrides, + }; +} + +function makeMetrics(sessionId = "session-1"): SessionDetailedMetrics { + return { + session_id: sessionId, + state: SessionState.ACTIVE, + is_alive: true, + created_at: NOW, + last_activity: NOW, + uptime_seconds: 1, + idle_seconds: 0, + commands: { total_executed: 1, current_count: 1, history_length: 1 }, + performance: { total_cpu_time: 0.1, peak_memory_mb: 10, current_memory_mb: 8 }, + buffer: { current_size: 0, max_size: 1024, utilization_percent: 0 }, + timeout: { configured_seconds: null, is_timed_out: false }, + }; +} + +function makeManagerMetrics(): ManagerMetrics { + return { + manager: { total_sessions: 1, active_sessions: 1, terminated_sessions: 0, max_sessions: 50, utilization_percent: 2 }, + aggregate: { total_commands: 5, total_cpu_time: 0.5, total_memory_mb: 8, avg_memory_per_session: 8 }, + sessions: [makeMetrics()], + }; +} + +interface MockManager extends Record { + createSession: Mock; + executeCommand: Mock; + listSessions: Mock; + getSessionInfo: Mock; + terminateSession: Mock; + inspectSession: Mock; + getSessionHistory: Mock; + sessionMetrics: Mock; +} + +function makeMockManager(): MockManager { + return { + createSession: vi.fn().mockResolvedValue("session-1"), + executeCommand: vi.fn().mockResolvedValue(makeExecResult()), + listSessions: vi.fn().mockResolvedValue([]), + getSessionInfo: vi.fn().mockResolvedValue(makeSessionInfo()), + terminateSession: vi.fn().mockResolvedValue(undefined), + inspectSession: vi.fn().mockResolvedValue(makeMetrics()), + getSessionHistory: vi.fn().mockResolvedValue([]), + sessionMetrics: vi.fn().mockResolvedValue(makeManagerMetrics()), + }; +} + +// --------------------------------------------------------------------------- +// QueryShellTool +// --------------------------------------------------------------------------- + +describe("QueryShellTool", () => { + let mgr: MockManager; + let tool: QueryShellTool; + + beforeEach(() => { + mgr = makeMockManager(); + tool = new QueryShellTool(mgr as unknown as OpenROADManager); + }); + + it("auto-creates a session when sessionId is null", async () => { + const raw = await tool.execute("help", null); + const result = JSON.parse(raw); + expect(mgr.createSession).toHaveBeenCalledOnce(); + expect(result.output).toBe("test output"); + expect(result.session_id).toBe("session-1"); + }); + + it("uses an existing session without creating a new one", async () => { + const raw = await tool.execute("help", "session-1"); + JSON.parse(raw); + expect(mgr.createSession).not.toHaveBeenCalled(); + expect(mgr.executeCommand).toHaveBeenCalledWith("session-1", "help", undefined); + }); + + it("returns snake_case keys in JSON output", async () => { + const raw = await tool.execute("help", "session-1"); + const result = JSON.parse(raw); + expect(Object.keys(result)).toContain("session_id"); + expect(Object.keys(result)).toContain("execution_time"); + expect(Object.keys(result)).toContain("command_count"); + expect(Object.keys(result)).toContain("buffer_size"); + }); + + it("handles SessionNotFoundError", async () => { + mgr.executeCommand.mockRejectedValue(new SessionNotFoundError("not found", "session-1")); + const raw = await tool.execute("help", "session-1"); + const result = JSON.parse(raw); + expect(result.output).toBe("Error: Session 'session-1' not found."); + expect(result.error).toContain("not found"); + }); + + it("handles SessionTerminatedError", async () => { + mgr.executeCommand.mockRejectedValue(new SessionTerminatedError("terminated", "session-1")); + const raw = await tool.execute("help", "session-1"); + const result = JSON.parse(raw); + expect(result.output).toBe(""); + expect(result.error).toContain("terminated"); + }); + + it("handles unexpected errors", async () => { + mgr.executeCommand.mockRejectedValue(new Error("boom")); + const raw = await tool.execute("help", "session-1"); + const result = JSON.parse(raw); + expect(result.error).toContain("Unexpected error"); + expect(result.error).toContain("boom"); + }); + + it("blocks dangerous commands when whitelist is enabled", async () => { + // `quit` is in BLOCKED_COMMANDS + const raw = await tool.execute("quit"); + const result = JSON.parse(raw); + expect(result.error).toMatch(/CommandBlocked/); + expect(mgr.executeCommand).not.toHaveBeenCalled(); + }); + + it("InteractiveShellTool is an alias for QueryShellTool", () => { + expect(InteractiveShellTool).toBe(QueryShellTool); + }); +}); + +// --------------------------------------------------------------------------- +// ExecShellTool +// --------------------------------------------------------------------------- + +describe("ExecShellTool", () => { + let mgr: MockManager; + let tool: ExecShellTool; + + beforeEach(() => { + mgr = makeMockManager(); + tool = new ExecShellTool(mgr as unknown as OpenROADManager); + }); + + it("executes a state-modifying command", async () => { + const raw = await tool.execute("set_wire_rc -signal -layer metal3", "session-1"); + const result = JSON.parse(raw); + expect(result.output).toBe("test output"); + expect(mgr.createSession).not.toHaveBeenCalled(); + }); + + it("blocks quit via BLOCKED_COMMANDS", async () => { + const raw = await tool.execute("quit"); + const result = JSON.parse(raw); + expect(result.error).toMatch(/CommandBlocked/); + }); + + it("handles SessionNotFoundError", async () => { + mgr.executeCommand.mockRejectedValue(new SessionNotFoundError("missing", "session-1")); + const raw = await tool.execute("read_lef foo.lef", "session-1"); + const result = JSON.parse(raw); + expect(result.output).toContain("not found"); + }); +}); + +// --------------------------------------------------------------------------- +// ListSessionsTool +// --------------------------------------------------------------------------- + +describe("ListSessionsTool", () => { + let mgr: MockManager; + let tool: ListSessionsTool; + + beforeEach(() => { + mgr = makeMockManager(); + tool = new ListSessionsTool(mgr as unknown as OpenROADManager); + }); + + it("returns empty list when no sessions exist", async () => { + const raw = await tool.execute(); + const result = JSON.parse(raw); + expect(result.sessions).toEqual([]); + expect(result.total_count).toBe(0); + expect(result.active_count).toBe(0); + expect(result.error).toBeNull(); + }); + + it("counts only alive sessions in active_count", async () => { + mgr.listSessions.mockResolvedValue([ + makeSessionInfo({ sessionId: "s1", isAlive: true }), + makeSessionInfo({ sessionId: "s2", isAlive: false }), + makeSessionInfo({ sessionId: "s3", isAlive: true }), + ]); + const raw = await tool.execute(); + const result = JSON.parse(raw); + expect(result.total_count).toBe(3); + expect(result.active_count).toBe(2); + }); + + it("returns error field on exception", async () => { + mgr.listSessions.mockRejectedValue(new Error("db error")); + const raw = await tool.execute(); + const result = JSON.parse(raw); + expect(result.error).toBeTruthy(); + expect(result.sessions).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// CreateSessionTool +// --------------------------------------------------------------------------- + +describe("CreateSessionTool", () => { + let mgr: MockManager; + let tool: CreateSessionTool; + + beforeEach(() => { + mgr = makeMockManager(); + tool = new CreateSessionTool(mgr as unknown as OpenROADManager); + }); + + it("creates a session with default parameters", async () => { + const raw = await tool.execute(); + const result = JSON.parse(raw); + expect(mgr.createSession).toHaveBeenCalledOnce(); + expect(result.session_id).toBe("session-1"); + expect(result.is_alive).toBe(true); + }); + + it("passes custom parameters to createSession", async () => { + await tool.execute("my-id", ["openroad"], { KEY: "VAL" }, "/tmp"); + expect(mgr.createSession).toHaveBeenCalledWith({ + sessionId: "my-id", + command: ["openroad"], + env: { KEY: "VAL" }, + cwd: "/tmp", + }); + }); + + it("returns error info when creation fails", async () => { + mgr.createSession.mockRejectedValue(new SessionError("limit reached")); + const raw = await tool.execute("my-id"); + const result = JSON.parse(raw); + expect(result.is_alive).toBe(false); + expect(result.error).toContain("limit reached"); + }); +}); + +// --------------------------------------------------------------------------- +// TerminateSessionTool +// --------------------------------------------------------------------------- + +describe("TerminateSessionTool", () => { + let mgr: MockManager; + let tool: TerminateSessionTool; + + beforeEach(() => { + mgr = makeMockManager(); + tool = new TerminateSessionTool(mgr as unknown as OpenROADManager); + }); + + it("terminates a session normally", async () => { + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.terminated).toBe(true); + expect(result.was_alive).toBe(true); + expect(result.force).toBe(false); + expect(result.error).toBeNull(); + }); + + it("force-terminates a session", async () => { + const raw = await tool.execute("session-1", true); + const result = JSON.parse(raw); + expect(result.force).toBe(true); + expect(result.terminated).toBe(true); + }); + + it("handles terminating a non-existent session", async () => { + mgr.getSessionInfo.mockRejectedValue(new SessionNotFoundError("not found", "session-1")); + mgr.terminateSession.mockRejectedValue(new SessionNotFoundError("not found", "session-1")); + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.terminated).toBe(false); + expect(result.error).toBeTruthy(); + }); + + it("handles unexpected termination errors", async () => { + mgr.terminateSession.mockRejectedValue(new Error("PTY crash")); + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.terminated).toBe(false); + expect(result.error).toContain("Termination failed"); + }); +}); + +// --------------------------------------------------------------------------- +// InspectSessionTool +// --------------------------------------------------------------------------- + +describe("InspectSessionTool", () => { + let mgr: MockManager; + let tool: InspectSessionTool; + + beforeEach(() => { + mgr = makeMockManager(); + tool = new InspectSessionTool(mgr as unknown as OpenROADManager); + }); + + it("returns session metrics", async () => { + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.session_id).toBe("session-1"); + expect(result.metrics).toBeTruthy(); + expect(result.metrics.session_id).toBe("session-1"); + expect(result.error).toBeNull(); + }); + + it("returns error when session not found", async () => { + mgr.inspectSession.mockRejectedValue(new SessionNotFoundError("missing", "session-1")); + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.metrics).toBeNull(); + expect(result.error).toBeTruthy(); + }); + + it("returns error on unexpected failure", async () => { + mgr.inspectSession.mockRejectedValue(new Error("cpu panic")); + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.error).toContain("Inspection failed"); + }); +}); + +// --------------------------------------------------------------------------- +// SessionHistoryTool +// --------------------------------------------------------------------------- + +describe("SessionHistoryTool", () => { + let mgr: MockManager; + let tool: SessionHistoryTool; + + beforeEach(() => { + mgr = makeMockManager(); + tool = new SessionHistoryTool(mgr as unknown as OpenROADManager); + }); + + it("returns session history", async () => { + mgr.getSessionHistory.mockResolvedValue([ + { command: "help", timestamp: NOW, command_number: 1, execution_start: 0 }, + ]); + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.session_id).toBe("session-1"); + expect(result.total_commands).toBe(1); + expect(result.history).toHaveLength(1); + expect(result.error).toBeNull(); + }); + + it("passes limit and search parameters", async () => { + await tool.execute("session-1", 10, "report"); + expect(mgr.getSessionHistory).toHaveBeenCalledWith("session-1", 10, "report"); + }); + + it("returns error when session not found", async () => { + mgr.getSessionHistory.mockRejectedValue(new SessionNotFoundError("gone", "session-1")); + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.history).toEqual([]); + expect(result.total_commands).toBe(0); + expect(result.error).toBeTruthy(); + }); + + it("returns error on unexpected failure", async () => { + mgr.getSessionHistory.mockRejectedValue(new Error("disk full")); + const raw = await tool.execute("session-1"); + const result = JSON.parse(raw); + expect(result.error).toContain("History retrieval failed"); + }); +}); + +// --------------------------------------------------------------------------- +// SessionMetricsTool +// --------------------------------------------------------------------------- + +describe("SessionMetricsTool", () => { + let mgr: MockManager; + let tool: SessionMetricsTool; + + beforeEach(() => { + mgr = makeMockManager(); + tool = new SessionMetricsTool(mgr as unknown as OpenROADManager); + }); + + it("returns manager-wide metrics", async () => { + const raw = await tool.execute(); + const result = JSON.parse(raw); + expect(result.metrics).toBeTruthy(); + expect(result.metrics.manager.total_sessions).toBe(1); + expect(result.error).toBeNull(); + }); + + it("returns error on unexpected failure", async () => { + mgr.sessionMetrics.mockRejectedValue(new Error("overload")); + const raw = await tool.execute(); + const result = JSON.parse(raw); + expect(result.metrics).toBeNull(); + expect(result.error).toContain("Metrics retrieval failed"); + }); +}); + +// --------------------------------------------------------------------------- +// Integration: full workflow +// --------------------------------------------------------------------------- + +describe("Integration: session workflow", () => { + it("create → execute → list → terminate", async () => { + const mgr = makeMockManager(); + mgr.listSessions.mockResolvedValue([makeSessionInfo()]); + + const created = JSON.parse(await new CreateSessionTool(mgr as unknown as OpenROADManager).execute("test-id")); + expect(created.session_id).toBe("session-1"); + + const exec = JSON.parse(await new QueryShellTool(mgr as unknown as OpenROADManager).execute("help", "session-1")); + expect(exec.output).toBe("test output"); + + const list = JSON.parse(await new ListSessionsTool(mgr as unknown as OpenROADManager).execute()); + expect(list.total_count).toBe(1); + + const term = JSON.parse(await new TerminateSessionTool(mgr as unknown as OpenROADManager).execute("session-1")); + expect(term.terminated).toBe(true); + }); + + it("concurrent operations complete without interference", async () => { + const mgr = makeMockManager(); + const queryTool = new QueryShellTool(mgr as unknown as OpenROADManager); + const [r1, r2, r3] = await Promise.all([ + queryTool.execute("help", "session-1"), + queryTool.execute("version", "session-1"), + queryTool.execute("report_checks", "session-1"), + ]); + expect(JSON.parse(r1).output).toBe("test output"); + expect(JSON.parse(r2).output).toBe("test output"); + expect(JSON.parse(r3).output).toBe("test output"); + }); +}); + +// --------------------------------------------------------------------------- +// Snapshot: one representative output per tool +// --------------------------------------------------------------------------- + +describe("Snapshots: wire format stability", () => { + it("QueryShellTool success output", async () => { + const mgr = makeMockManager(); + const raw = await new QueryShellTool(mgr as unknown as OpenROADManager).execute("help", "session-1"); + expect(raw).toMatchSnapshot(); + }); + + it("ListSessionsTool with sessions", async () => { + const mgr = makeMockManager(); + mgr.listSessions.mockResolvedValue([makeSessionInfo()]); + const raw = await new ListSessionsTool(mgr as unknown as OpenROADManager).execute(); + expect(raw).toMatchSnapshot(); + }); +}); From deb03173da1c16e9a7e9bb86c1febe8680b1bcb8 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Tue, 16 Jun 2026 21:24:51 -0600 Subject: [PATCH 06/44] add tests for report image tools covering path traversal and platform validation --- .../__tests__/tools/report_images.test.ts | 443 ++++++++++++++++++ 1 file changed, 443 insertions(+) create mode 100644 typescript/__tests__/tools/report_images.test.ts diff --git a/typescript/__tests__/tools/report_images.test.ts b/typescript/__tests__/tools/report_images.test.ts new file mode 100644 index 0000000..1485588 --- /dev/null +++ b/typescript/__tests__/tools/report_images.test.ts @@ -0,0 +1,443 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { + classifyImageType, + ListReportImagesTool, + ReadReportImageTool, + validatePlatformDesign, +} from "../../src/tools/report_images.js"; +import type { OpenROADManager } from "../../src/core/manager.js"; + +// --------------------------------------------------------------------------- +// Mock getSettings so tests don't depend on the filesystem ORFS installation +// --------------------------------------------------------------------------- + +vi.mock("../../src/config/settings.js", () => { + let mockFlowPath = "/mock/flow"; + let mockPlatforms: string[] = []; + let mockDesigns: Record = {}; + return { + getSettings: vi.fn(() => ({ + get flowPath() { return mockFlowPath; }, + get platforms() { return mockPlatforms; }, + designs(platform: string) { return mockDesigns[platform] ?? []; }, + WHITELIST_ENABLED: false, + LOG_LEVEL: "INFO", + COMMAND_TIMEOUT: 30, + COMMAND_COMPLETION_DELAY: 0.1, + DEFAULT_BUFFER_SIZE: 131072, + MAX_SESSIONS: 50, + SESSION_QUEUE_SIZE: 128, + SESSION_IDLE_TIMEOUT: 300, + READ_CHUNK_SIZE: 8192, + LOG_FORMAT: "", + ALLOWED_COMMANDS: ["openroad"], + ENABLE_COMMAND_VALIDATION: true, + ORFS_FLOW_PATH: "/mock/flow", + })), + __setMock(fp: string, plats: string[], des: Record) { + mockFlowPath = fp; + mockPlatforms = plats; + mockDesigns = des; + }, + }; +}); + +import { getSettings } from "../../src/config/settings.js"; + +// --------------------------------------------------------------------------- +// Fixture helpers +// --------------------------------------------------------------------------- + +let tmpDir: string; + +function createFixture( + platform = "nangate45", + design = "gcd", + runSlug = "run-123", + imageFiles: string[] = ["cts_clk.webp", "final_all.webp"], +) { + const flowPath = tmpDir; + // Settings directories (used by platforms / designs accessors) + fs.mkdirSync(path.join(flowPath, "platforms", platform), { recursive: true }); + fs.mkdirSync(path.join(flowPath, "designs", platform, design), { recursive: true }); + // Reports directory + const runPath = path.join(flowPath, "reports", platform, design, runSlug); + fs.mkdirSync(runPath, { recursive: true }); + for (const img of imageFiles) { + fs.writeFileSync(path.join(runPath, img), Buffer.from("RIFF\x00\x00\x00\x00WEBP")); + } + return { flowPath, runPath }; +} + +// Stub manager (tools don't call manager methods directly, but constructor requires it) +const stubManager = {} as unknown as OpenROADManager; + +// --------------------------------------------------------------------------- + +beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openroad-test-")); +}); + +afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + vi.clearAllMocks(); +}); + +// --------------------------------------------------------------------------- +// classifyImageType +// --------------------------------------------------------------------------- + +describe("classifyImageType", () => { + it("classifies CTS images correctly", () => { + expect(classifyImageType("cts_clk.webp")).toEqual(["cts", "clock_visualization"]); + expect(classifyImageType("cts_clk_layout.webp")).toEqual(["cts", "clock_layout"]); + expect(classifyImageType("cts_core_clock.webp")).toEqual(["cts", "core_clock_visualization"]); + }); + + it("classifies final stage images correctly", () => { + expect(classifyImageType("final_all.webp")).toEqual(["final", "complete_design"]); + expect(classifyImageType("final_congestion.webp")).toEqual(["final", "congestion_heatmap"]); + expect(classifyImageType("final_ir_drop.webp")).toEqual(["final", "ir_drop_analysis"]); + }); + + it("returns unknown for unrecognised filenames", () => { + expect(classifyImageType("unknown_image.webp")).toEqual(["unknown", "unknown"]); + expect(classifyImageType("foo.webp")).toEqual(["unknown", "unknown"]); + }); + + it("returns unknown stage when filename has no underscore", () => { + const [stage, _type] = classifyImageType("nounderscore.webp"); + expect(stage).toBe("unknown"); + }); +}); + +// --------------------------------------------------------------------------- +// validatePlatformDesign +// --------------------------------------------------------------------------- + +describe("validatePlatformDesign", () => { + it("throws on unknown platform", () => { + (getSettings as ReturnType).mockReturnValueOnce({ + platforms: ["nangate45"], + designs: () => ["gcd"], + flowPath: tmpDir, + }); + expect(() => validatePlatformDesign("bad_platform", "gcd")).toThrow(); + }); + + it("throws on unknown design", () => { + (getSettings as ReturnType).mockReturnValueOnce({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath: tmpDir, + }); + expect(() => validatePlatformDesign("nangate45", "bad_design")).toThrow(); + }); +}); + +// --------------------------------------------------------------------------- +// ListReportImagesTool +// --------------------------------------------------------------------------- + +describe("ListReportImagesTool", () => { + let tool: ListReportImagesTool; + + beforeEach(() => { + tool = new ListReportImagesTool(stubManager); + }); + + it("returns error when platform is invalid", async () => { + (getSettings as ReturnType).mockReturnValueOnce({ + platforms: [], + designs: () => [], + flowPath: tmpDir, + }); + const raw = await tool.execute("bad_platform", "gcd", "run-123"); + const result = JSON.parse(raw); + expect(result.error).toBeTruthy(); + }); + + it("returns RunNotFound error when run directory does not exist", async () => { + const { flowPath } = createFixture(); + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath, + WHITELIST_ENABLED: false, + }); + const raw = await tool.execute("nangate45", "gcd", "nonexistent"); + const result = JSON.parse(raw); + expect(result.error).toBe("RunNotFound"); + }); + + it("returns totalImages 0 when run directory has no .webp files", async () => { + const { flowPath, runPath } = createFixture("nangate45", "gcd", "run-empty", []); + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath, + WHITELIST_ENABLED: false, + }); + const raw = await tool.execute("nangate45", "gcd", "run-empty"); + const result = JSON.parse(raw); + expect(result.total_images).toBe(0); + expect(result.images_by_stage).toEqual({}); + }); + + it("lists all .webp files grouped by stage", async () => { + const { flowPath } = createFixture(); + (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(2); + expect(result.images_by_stage).toBeTruthy(); + expect(result.images_by_stage).toHaveProperty("cts"); + expect(result.images_by_stage).toHaveProperty("final"); + }); + + it("filters images by stage", async () => { + const { flowPath } = createFixture("nangate45", "gcd", "run-123", [ + "cts_clk.webp", + "final_all.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", "cts"); + const result = JSON.parse(raw); + expect(result.total_images).toBe(1); + expect(result.images_by_stage).toHaveProperty("cts"); + expect(result.images_by_stage).not.toHaveProperty("final"); + }); +}); + +// --------------------------------------------------------------------------- +// ReadReportImageTool +// --------------------------------------------------------------------------- + +describe("ReadReportImageTool", () => { + let tool: ReadReportImageTool; + + beforeEach(() => { + tool = new ReadReportImageTool(stubManager); + }); + + it("returns error when platform is invalid", async () => { + (getSettings as ReturnType).mockReturnValueOnce({ + platforms: [], + designs: () => [], + flowPath: tmpDir, + }); + const raw = await tool.execute("bad_platform", "gcd", "run-123", "cts_clk.webp"); + const result = JSON.parse(raw); + expect(result.error).toBeTruthy(); + expect(result.image_data).toBeNull(); + }); + + it("returns RunNotFound when run directory does not exist", async () => { + const { flowPath } = createFixture(); + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath, + WHITELIST_ENABLED: false, + }); + const raw = await tool.execute("nangate45", "gcd", "missing-run", "cts_clk.webp"); + const result = JSON.parse(raw); + expect(result.error).toBe("RunNotFound"); + }); + + it("returns ImageNotFound when image does not exist", async () => { + const { flowPath } = createFixture(); + (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", "missing.webp"); + const result = JSON.parse(raw); + expect(result.error).toBe("ImageNotFound"); + }); + + it("reads and base64-encodes a .webp image successfully", async () => { + const { flowPath } = createFixture("nangate45", "gcd", "run-123", ["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", "cts_clk.webp"); + const result = JSON.parse(raw); + // Should have image_data as a base64 string + expect(typeof result.image_data).toBe("string"); + expect(result.image_data.length).toBeGreaterThan(0); + // Round-trip check + const decoded = Buffer.from(result.image_data, "base64"); + expect(decoded.length).toBeGreaterThan(0); + // Metadata presence + expect(result.metadata).toBeTruthy(); + expect(result.metadata.filename).toBe("cts_clk.webp"); + expect(result.metadata.stage).toBe("cts"); + expect(result.metadata.type).toBe("clock_visualization"); + }); + + it("returns FileTooLarge error when image exceeds 50 MB", async () => { + const { flowPath, runPath } = createFixture("nangate45", "gcd", "run-123", []); + // Write a "file" that appears to be 51 MB by mocking statSync + const bigPath = path.join(runPath, "huge.webp"); + fs.writeFileSync(bigPath, Buffer.from("tiny content")); + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath, + WHITELIST_ENABLED: false, + }); + const originalStatSync = fs.statSync.bind(fs); + const statSpy = vi.spyOn(fs, "statSync").mockImplementation((p) => { + if (p === bigPath) return { size: 51 * 1024 * 1024, isFile: () => true, mtime: new Date() } as unknown as fs.Stats; + return originalStatSync(p) as fs.Stats; + }); + const raw = await tool.execute("nangate45", "gcd", "run-123", "huge.webp"); + const result = JSON.parse(raw); + expect(result.error).toBe("FileTooLarge"); + statSpy.mockRestore(); + }); + + it("rejects non-.webp extension", async () => { + const { flowPath } = createFixture(); + (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", "cts_clk.png"); + const result = JSON.parse(raw); + expect(result.error).toBe("InvalidImageName"); + }); +}); + +// --------------------------------------------------------------------------- +// TestPathTraversalSecurity +// --------------------------------------------------------------------------- + +describe("TestPathTraversalSecurity", () => { + let tool: ListReportImagesTool; + let readTool: ReadReportImageTool; + let flowPath: string; + + beforeEach(() => { + const fixture = createFixture(); + flowPath = fixture.flowPath; + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath, + WHITELIST_ENABLED: false, + }); + tool = new ListReportImagesTool(stubManager); + readTool = new ReadReportImageTool(stubManager); + }); + + it("rejects path traversal in run_slug (../../etc/passwd)", async () => { + const raw = await tool.execute("nangate45", "gcd", "../../../etc/passwd"); + const result = JSON.parse(raw); + expect(result.error).toBeTruthy(); + expect(result.error).not.toBe("RunNotFound"); // must be a validation error + }); + + it("rejects bare '..' as run_slug", async () => { + const raw = await tool.execute("nangate45", "gcd", ".."); + const result = JSON.parse(raw); + expect(result.error).toBeTruthy(); + }); + + it("rejects glob characters in run_slug", async () => { + const raw = await tool.execute("nangate45", "gcd", "*"); + const result = JSON.parse(raw); + expect(result.error).toBeTruthy(); + }); + + it("rejects path traversal in image_name", async () => { + const raw = await readTool.execute("nangate45", "gcd", "run-123", "../../../etc/passwd"); + const result = JSON.parse(raw); + expect(result.error).toBeTruthy(); + }); + + it("rejects non-.webp extension in image_name", async () => { + const raw = await readTool.execute("nangate45", "gcd", "run-123", "file.sh"); + const result = JSON.parse(raw); + expect(result.error).toBe("InvalidImageName"); + }); + + it("rejects null byte in image_name", async () => { + const raw = await readTool.execute("nangate45", "gcd", "run-123", "evil\x00.webp"); + const result = JSON.parse(raw); + expect(result.error).toBeTruthy(); + }); + + it("blocks symlink escape from run directory", async () => { + // Create a symlink inside run-123 that points outside + const runPath = path.join(flowPath, "reports", "nangate45", "gcd", "run-123"); + const linkPath = path.join(runPath, "escape.webp"); + try { + fs.symlinkSync("/etc/passwd", linkPath); + } catch { + // symlink creation may fail in some environments — skip gracefully + return; + } + const raw = await readTool.execute("nangate45", "gcd", "run-123", "escape.webp"); + const result = JSON.parse(raw); + // Should either not find the image, reject path containment, or return an error + // — but must NOT return valid image_data that resolves to /etc/passwd content + expect(result.image_data === null || result.error !== null).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// TestPlatformDesignValidationInTools +// --------------------------------------------------------------------------- + +describe("TestPlatformDesignValidationInTools", () => { + beforeEach(() => { + (getSettings as ReturnType).mockReturnValue({ + platforms: ["nangate45"], + designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), + flowPath: tmpDir, + WHITELIST_ENABLED: false, + }); + }); + + it("list tool returns error for invalid platform", async () => { + const raw = await new ListReportImagesTool(stubManager).execute("invalid_plat", "gcd", "run-123"); + expect(JSON.parse(raw).error).toBeTruthy(); + }); + + it("list tool returns error for invalid design", async () => { + const raw = await new ListReportImagesTool(stubManager).execute("nangate45", "bad_design", "run-123"); + expect(JSON.parse(raw).error).toBeTruthy(); + }); + + it("read tool returns error for invalid platform", async () => { + const raw = await new ReadReportImageTool(stubManager).execute("invalid_plat", "gcd", "run-123", "img.webp"); + expect(JSON.parse(raw).error).toBeTruthy(); + }); + + it("read tool returns error for invalid design", async () => { + const raw = await new ReadReportImageTool(stubManager).execute("nangate45", "bad_design", "run-123", "img.webp"); + expect(JSON.parse(raw).error).toBeTruthy(); + }); +}); From 2cd26591ebd38b0d9af832ac81ff471dbf713bb2 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Tue, 16 Jun 2026 21:40:01 -0600 Subject: [PATCH 07/44] verify process liveness with kill(0) so a missed exit event cannot leave the pty marked alive --- typescript/src/interactive/pty_handler.ts | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index 0061a1e..9d2a356 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -89,17 +89,21 @@ export class PtyHandler { this._alive = true; this._exitCode = null; - if (onData) { - this._dataDisposable = this._ptyProcess.onData(onData); - } - + // Register the exit handler before onData so a fast-exiting process can + // never slip its exit event through before we are listening. The guard + // keeps the handler idempotent against a double-delivered exit. this._exitDisposable = this._ptyProcess.onExit(({ exitCode }) => { + if (!this._alive && this._exitCode !== null) return; this._alive = false; this._exitCode = exitCode; const resolvers = this._exitResolvers.splice(0); for (const resolve of resolvers) resolve(exitCode); onExit?.(exitCode); }); + + if (onData) { + this._dataDisposable = this._ptyProcess.onData(onData); + } } catch (e) { if (e instanceof PTYError) throw e; throw new PTYError(`Failed to create PTY session: ${e}`); @@ -118,7 +122,16 @@ export class PtyHandler { } isProcessAlive(): boolean { - return this._alive; + if (!this._alive || !this._ptyProcess) return false; + // Defensive liveness probe: if the exit event was somehow missed, signal 0 + // detects a dead/reaped pid (ESRCH) so `_alive` cannot stay true forever. + try { + process.kill(this._ptyProcess.pid, 0); + return true; + } catch { + this._alive = false; + return false; + } } async waitForExit(timeoutMs?: number): Promise { From 4631ed567d3f4cf0489f61cb769aabce3fc39c60 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Tue, 16 Jun 2026 21:41:54 -0600 Subject: [PATCH 08/44] serialize session terminate and cleanup with a lifecycle lock to prevent double kill and stale exit codes --- typescript/src/interactive/session.ts | 46 ++++++++++++++++----------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index ef807df..bcf27d3 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -1,4 +1,5 @@ import pidusage from "pidusage"; +import { Mutex } from "async-mutex"; import { ANSIDecoder } from "../utils/ansi_decoder.js"; import { getSettings } from "../config/settings.js"; import type { Settings } from "../config/settings.js"; @@ -55,6 +56,9 @@ export class InteractiveSession { private _inputWaiters: Array<() => void> = []; private _isShutdown = false; private _writerTask: Promise | null = null; + // Serialises terminate()/cleanup() so concurrent callers cannot double-kill + // the process or deliver a stale exit code to waiters. + private readonly _lifecycleLock = new Mutex(); constructor(sessionId: string, bufferSize?: number, private readonly _settings: Settings = getSettings()) { this.sessionId = sessionId; @@ -261,33 +265,37 @@ export class InteractiveSession { } async terminate(force = false): Promise { - if (this._state === SessionState.TERMINATED) return; + await this._lifecycleLock.runExclusive(async () => { + if (this._state === SessionState.TERMINATED) return; - this._state = SessionState.TERMINATED; - this._signalShutdown(); + this._state = SessionState.TERMINATED; + this._signalShutdown(); - await this.pty.terminateProcess(force); - await this.pty.cleanup(); + await this.pty.terminateProcess(force); + await this.pty.cleanup(); - if (this._writerTask !== null) { - await this._writerTask; - this._writerTask = null; - } + if (this._writerTask !== null) { + await this._writerTask; + this._writerTask = null; + } + }); } async cleanup(): Promise { - if (this._state !== SessionState.TERMINATED && this._state !== SessionState.ERROR) { - this._state = SessionState.TERMINATED; - } - this._signalShutdown(); + await this._lifecycleLock.runExclusive(async () => { + if (this._state !== SessionState.TERMINATED && this._state !== SessionState.ERROR) { + this._state = SessionState.TERMINATED; + } + this._signalShutdown(); - if (this._writerTask !== null) { - await this._writerTask; - this._writerTask = null; - } + if (this._writerTask !== null) { + await this._writerTask; + this._writerTask = null; + } - await this.pty.cleanup(); - await this.outputBuffer.clear(); + await this.pty.cleanup(); + await this.outputBuffer.clear(); + }); } private _signalShutdown(): void { From 9a5257d89d0fdd83f0c80095a5ac112f45716c22 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 06:39:47 -0600 Subject: [PATCH 09/44] drop redundant cleanup call in terminateSession so final output buffer is not discarded --- typescript/__tests__/core/manager.test.ts | 4 +++- typescript/src/core/manager.ts | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/typescript/__tests__/core/manager.test.ts b/typescript/__tests__/core/manager.test.ts index 07c7982..0e28a20 100644 --- a/typescript/__tests__/core/manager.test.ts +++ b/typescript/__tests__/core/manager.test.ts @@ -187,7 +187,9 @@ describe("OpenROADManager", () => { await manager.createSession({ sessionId: "s1" }); await manager.terminateSession("s1", true); expect(created[0]!.terminate).toHaveBeenCalledWith(true); - expect(created[0]!.cleanup).toHaveBeenCalledOnce(); + // terminate() handles teardown; cleanup() must not be called again here + // (it would clear the buffer and double-tear-down the PTY). + expect(created[0]!.cleanup).not.toHaveBeenCalled(); expect(manager.getSessionCount()).toBe(0); }); }); diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index d2276b7..fbe96a2 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -121,8 +121,11 @@ export class OpenROADManager { async terminateSession(sessionId: string, force = false): Promise { const session = this._getSession(sessionId); + // terminate() already tears down the PTY and stops the writer task. We do + // not also call cleanup() here: cleanup() clears the output buffer, which + // would discard final output a concurrent reader may still need. The + // session is dropped from the map below, so its buffer is GC'd anyway. await session.terminate(force); - await session.cleanup(); this.logger.info(`Terminated session ${sessionId}`); await this.cleanupLock.runExclusive(() => { From da7a25bd1687af049bce80ff158b4b3cb6299303 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 06:43:45 -0600 Subject: [PATCH 10/44] use function replacement in annotate and preserve modes so $& in escape sequences is not reinterpreted --- typescript/__tests__/utils/ansi_decoder.test.ts | 16 ++++++++++++++++ typescript/src/utils/ansi_decoder.ts | 9 +++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/typescript/__tests__/utils/ansi_decoder.test.ts b/typescript/__tests__/utils/ansi_decoder.test.ts index ba7fd2a..2731213 100644 --- a/typescript/__tests__/utils/ansi_decoder.test.ts +++ b/typescript/__tests__/utils/ansi_decoder.test.ts @@ -67,6 +67,22 @@ describe("ANSIDecoder", () => { }); }); + describe("translateOutput - $ replacement safety", () => { + it("does not reinterpret $& inside an OSC sequence body when annotating", () => { + // OSC title set with a literal "$&" in the body. With a string replacement + // the "$&" would expand to the matched sequence and corrupt the output. + const osc = "\x1b]0;$&title\x07"; + const result = ANSIDecoder.translateOutput(`before${osc}after`, "annotate"); + expect(result).toBe("before[Unknown escape sequence (\x1b]0;$&title\x07)]after"); + }); + + it("inserts the original sequence literally in preserve mode", () => { + const osc = "\x1b]0;$$x\x07"; + const result = ANSIDecoder.translateOutput(osc, "preserve"); + expect(result).toContain(osc); + }); + }); + describe("translateOutput - decode mode", () => { it("includes breakdown header", () => { const result = ANSIDecoder.translateOutput("\x1b[1mtext", "decode"); diff --git a/typescript/src/utils/ansi_decoder.ts b/typescript/src/utils/ansi_decoder.ts index 9e16ce4..91a2271 100644 --- a/typescript/src/utils/ansi_decoder.ts +++ b/typescript/src/utils/ansi_decoder.ts @@ -115,7 +115,11 @@ export class ANSIDecoder { if (mode === "annotate") { let result = text; for (const seq of new Set(sequences)) { - result = result.replaceAll(seq, `[${ANSIDecoder.decodeEscapeSequence(seq)}]`); + // Function replacement: the value is inserted literally, so `$&`/`$1` + // inside an OSC sequence body cannot be reinterpreted as a replacement + // pattern and corrupt the output. + const annotation = `[${ANSIDecoder.decodeEscapeSequence(seq)}]`; + result = result.replaceAll(seq, () => annotation); } return result.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); } @@ -123,7 +127,8 @@ export class ANSIDecoder { if (mode === "preserve") { let result = text; for (const seq of new Set(sequences)) { - result = result.replaceAll(seq, `${seq}[${ANSIDecoder.decodeEscapeSequence(seq)}]`); + const annotated = `${seq}[${ANSIDecoder.decodeEscapeSequence(seq)}]`; + result = result.replaceAll(seq, () => annotated); } return result; } From c3ae1d76cbc6bf9c086d621d33c3ca9a63fd97ff Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 06:45:12 -0600 Subject: [PATCH 11/44] transition session to terminated from any non-terminal state so a startup failure cannot leave a creating zombie --- typescript/src/interactive/session.ts | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index bcf27d3..551da81 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -115,9 +115,7 @@ export class InteractiveSession { // circular buffer's eviction logic bounds memory correctly. const appendChunk = (chunk: string): void => { this.outputBuffer.append(chunk).catch(() => { - if (this._state === SessionState.ACTIVE) { - this._state = SessionState.TERMINATED; - } + this._markDead(); this._signalShutdown(); }); }; @@ -138,7 +136,12 @@ export class InteractiveSession { }, ); - this._state = SessionState.ACTIVE; + // Only promote to ACTIVE if the session is still creating. A fast process + // death during startup may already have flipped us to TERMINATED via the + // onData/onExit handlers; do not resurrect it into an undead ACTIVE state. + if (this._state === SessionState.CREATING) { + this._state = SessionState.ACTIVE; + } this._writerTask = this._writeInput(); } catch (e) { this._state = SessionState.ERROR; @@ -304,6 +307,17 @@ export class InteractiveSession { for (const w of waiters) w(); } + /** + * Transition to TERMINATED from any non-terminal state (idempotent). Covers + * CREATING as well as ACTIVE so a session that dies mid-startup is never left + * stranded as an uncollectable CREATING zombie. + */ + private _markDead(): void { + if (this._state !== SessionState.TERMINATED && this._state !== SessionState.ERROR) { + this._state = SessionState.TERMINATED; + } + } + private async _writeInput(): Promise { while (!this._isShutdown) { const data = await this._dequeueInput(1000); @@ -312,9 +326,7 @@ export class InteractiveSession { try { this.pty.writeInput(data); } catch { - if (this._state === SessionState.ACTIVE) { - this._state = SessionState.TERMINATED; - } + this._markDead(); this._signalShutdown(); break; } From dfedfa4c2b9ddf7a5e526a29bd882470a596566c Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 06:57:09 -0600 Subject: [PATCH 12/44] ignore non-positive history limit so a negative value cannot drop recent commands --- typescript/__tests__/interactive/session.test.ts | 7 +++++++ typescript/src/interactive/session.ts | 5 +++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index a9a16bc..d1b4dcb 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -643,6 +643,13 @@ describe("InteractiveSession", () => { expect(limited[0]!.command).toBe("cmd_b"); }); + it("getCommandHistory ignores a negative limit instead of dropping entries", async () => { + await session.sendCommand("cmd_a"); + await session.sendCommand("cmd_b"); + const all = session.getCommandHistory(-1); + expect(all).toHaveLength(2); + }); + it("getDetailedMetrics returns the full nested shape", async () => { await session.sendCommand("report_wns"); const m = await session.getDetailedMetrics(); diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 551da81..937036c 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -459,8 +459,9 @@ export class InteractiveSession { // Sort by timestamp, most recent first. history.sort((a, b) => (a.timestamp < b.timestamp ? 1 : a.timestamp > b.timestamp ? -1 : 0)); - // Match Python's truthy check: limit === 0 leaves the list unsliced. - if (limit) { + // Only a positive limit slices. A zero or negative limit leaves the list + // intact rather than letting `slice(0, -n)` silently drop recent entries. + if (limit !== undefined && limit > 0) { history = history.slice(0, limit); } From 8c6203874ea84241ac1c2d346c244d307c470dc6 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:09:07 -0600 Subject: [PATCH 13/44] track session death time and use it for force-cleanup timing instead of last activity --- typescript/src/core/manager.ts | 7 ++++++- typescript/src/interactive/session.ts | 22 +++++++++++++++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index fbe96a2..9098243 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -290,7 +290,12 @@ export class OpenROADManager { for (const [sessionId, session] of this._initializedSessions()) { if (!session.isAlive()) { - const timeSinceDeath = (now - session.lastActivity.getTime()) / 1000; + // Measure from the actual death time. lastActivity is the last command, + // which for a long-idle session is far earlier and would trip the + // force-cleanup timer immediately. Fall back to lastActivity only if the + // death timestamp is somehow unset. + const deathTime = (session.terminatedAt ?? session.lastActivity).getTime(); + const timeSinceDeath = (now - deathTime) / 1000; terminated.push([sessionId, session, timeSinceDeath > FORCE_CLEANUP_AFTER_SECONDS]); } } diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 937036c..d14d8a6 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -49,6 +49,10 @@ export class InteractiveSession { sessionTimeoutSeconds: number | null = null; private _state: SessionState; + // Wall-clock time the process actually died, set on the first TERMINATED + // transition. Used by the manager's force-cleanup timer; lastActivity would + // be wrong because a long-idle session dies far after its last command. + private _terminatedAt: Date | null = null; pty: PtyHandler; readonly outputBuffer: CircularBuffer; @@ -73,15 +77,23 @@ export class InteractiveSession { } set state(value: SessionState) { + if (value === SessionState.TERMINATED && this._terminatedAt === null) { + this._terminatedAt = new Date(); + } this._state = value; } + /** Wall-clock time the session first became TERMINATED, or null if still alive. */ + get terminatedAt(): Date | null { + return this._terminatedAt; + } + isAlive(): boolean { if (this._state === SessionState.TERMINATED) return false; const processAlive = this.pty.isProcessAlive(); if (!processAlive && this._state === SessionState.ACTIVE) { - this._state = SessionState.TERMINATED; + this.state = SessionState.TERMINATED; this._signalShutdown(); return false; } @@ -130,7 +142,7 @@ export class InteractiveSession { }, (_exitCode: number) => { if (this._state !== SessionState.TERMINATED) { - this._state = SessionState.TERMINATED; + this.state = SessionState.TERMINATED; this._signalShutdown(); } }, @@ -271,7 +283,7 @@ export class InteractiveSession { await this._lifecycleLock.runExclusive(async () => { if (this._state === SessionState.TERMINATED) return; - this._state = SessionState.TERMINATED; + this.state = SessionState.TERMINATED; this._signalShutdown(); await this.pty.terminateProcess(force); @@ -287,7 +299,7 @@ export class InteractiveSession { async cleanup(): Promise { await this._lifecycleLock.runExclusive(async () => { if (this._state !== SessionState.TERMINATED && this._state !== SessionState.ERROR) { - this._state = SessionState.TERMINATED; + this.state = SessionState.TERMINATED; } this._signalShutdown(); @@ -314,7 +326,7 @@ export class InteractiveSession { */ private _markDead(): void { if (this._state !== SessionState.TERMINATED && this._state !== SessionState.ERROR) { - this._state = SessionState.TERMINATED; + this.state = SessionState.TERMINATED; } } From 7bbe2c50859175d161eb91023af8ef63e698a0c7 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:11:15 -0600 Subject: [PATCH 14/44] skip null session placeholders in terminateAllSessions so in-progress creates are not lost --- typescript/__tests__/core/manager.test.ts | 15 +++++++++++++++ typescript/src/core/manager.ts | 5 ++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/core/manager.test.ts b/typescript/__tests__/core/manager.test.ts index 0e28a20..e8e6338 100644 --- a/typescript/__tests__/core/manager.test.ts +++ b/typescript/__tests__/core/manager.test.ts @@ -202,6 +202,21 @@ describe("OpenROADManager", () => { expect(count).toBe(2); expect(manager.getSessionCount()).toBe(0); }); + + it("skips in-progress placeholders instead of throwing", async () => { + // A session whose start() never resolves leaves a null placeholder in the + // map (createSession holds the lock). terminateAllSessions must not try to + // terminate it (which would throw "still being created"). + MockedSession.mockImplementationOnce(function (this: unknown, sessionId: string) { + const mock = makeMockSession(sessionId); + mock.start.mockReturnValue(new Promise(() => {})); // never resolves + created.push(mock); + return mock as unknown as InteractiveSession; + } as unknown as (sessionId: string) => InteractiveSession); + void manager.createSession({ sessionId: "pending" }); + + await expect(manager.terminateAllSessions()).resolves.toBe(0); + }); }); describe("inspectSession & getSessionHistory", () => { diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index 9098243..f9325d7 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -134,7 +134,10 @@ export class OpenROADManager { } async terminateAllSessions(force = false): Promise { - const sessionIds = [...this.sessions.keys()]; + // Only initialized sessions are terminable. Null placeholders belong to an + // in-flight createSession (which resolves or removes them itself), so + // terminating them would throw "still being created" and be lost. + const sessionIds = this._initializedSessions().map(([sid]) => sid); if (sessionIds.length === 0) return 0; const results = await Promise.allSettled( From 222ef2e4d27dad61ff336a06e64c518ad66c96e0 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:12:19 -0600 Subject: [PATCH 15/44] reject infinite and negative float settings so a timeout cannot be silently disabled --- typescript/__tests__/config/settings.test.ts | 10 ++++++++++ typescript/src/config/settings.ts | 6 +++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/config/settings.test.ts b/typescript/__tests__/config/settings.test.ts index 1594a74..a901663 100644 --- a/typescript/__tests__/config/settings.test.ts +++ b/typescript/__tests__/config/settings.test.ts @@ -115,6 +115,16 @@ describe("Settings", () => { expect(() => Settings.fromEnv()).toThrow("OPENROAD_COMMAND_TIMEOUT"); }); + it("rejects Infinity for float fields", () => { + process.env["OPENROAD_COMMAND_TIMEOUT"] = "Infinity"; + expect(() => Settings.fromEnv()).toThrow("OPENROAD_COMMAND_TIMEOUT"); + }); + + it("rejects negative floats", () => { + process.env["OPENROAD_COMMAND_TIMEOUT"] = "-1"; + expect(() => Settings.fromEnv()).toThrow("OPENROAD_COMMAND_TIMEOUT"); + }); + it("throws on invalid int", () => { process.env["OPENROAD_MAX_SESSIONS"] = "3.7"; expect(() => Settings.fromEnv()).toThrow("OPENROAD_MAX_SESSIONS"); diff --git a/typescript/src/config/settings.ts b/typescript/src/config/settings.ts index 97fbbc6..638ba2f 100644 --- a/typescript/src/config/settings.ts +++ b/typescript/src/config/settings.ts @@ -18,7 +18,11 @@ function parseBool(envKey: string, val: string): boolean { function parseFloat_(envKey: string, val: string): number { if (val.trim() === "") throw new Error(`Invalid value for ${envKey}: (empty string). Expected float.`); const n = Number(val); - if (isNaN(n)) throw new Error(`Invalid value for ${envKey}: ${val}. Expected float.`); + // Reject NaN and Infinity: an infinite timeout/delay would disable the very + // limit it configures, and a negative duration is never meaningful here. + if (!Number.isFinite(n) || n < 0) { + throw new Error(`Invalid value for ${envKey}: ${val}. Expected a non-negative finite float.`); + } return n; } From 35c494627d5084a6265abdc55a6d4005553dea58 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:14:31 -0600 Subject: [PATCH 16/44] reject negative integer settings so a negative session limit cannot block all creation --- typescript/__tests__/config/settings.test.ts | 5 +++++ typescript/src/config/settings.ts | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/config/settings.test.ts b/typescript/__tests__/config/settings.test.ts index a901663..799cf97 100644 --- a/typescript/__tests__/config/settings.test.ts +++ b/typescript/__tests__/config/settings.test.ts @@ -130,6 +130,11 @@ describe("Settings", () => { expect(() => Settings.fromEnv()).toThrow("OPENROAD_MAX_SESSIONS"); }); + it("rejects negative integers for int fields", () => { + process.env["OPENROAD_MAX_SESSIONS"] = "-1"; + expect(() => Settings.fromEnv()).toThrow("OPENROAD_MAX_SESSIONS"); + }); + it("rejects decimal strings for int fields", () => { process.env["OPENROAD_MAX_SESSIONS"] = "50.0"; expect(() => Settings.fromEnv()).toThrow("OPENROAD_MAX_SESSIONS"); diff --git a/typescript/src/config/settings.ts b/typescript/src/config/settings.ts index 638ba2f..c48b383 100644 --- a/typescript/src/config/settings.ts +++ b/typescript/src/config/settings.ts @@ -29,7 +29,11 @@ function parseFloat_(envKey: string, val: string): number { function parseInt_(envKey: string, val: string): number { if (val.trim() === "") throw new Error(`Invalid value for ${envKey}: (empty string). Expected int.`); if (!/^-?\d+$/.test(val.trim())) throw new Error(`Invalid value for ${envKey}: ${val}. Expected int.`); - return Number(val); + const n = Number(val); + // Every integer setting (session/buffer/queue limits) must be non-negative; a + // negative value such as MAX_SESSIONS=-1 would block all session creation. + if (n < 0) throw new Error(`Invalid value for ${envKey}: ${val}. Expected a non-negative integer.`); + return n; } export class Settings { From 51379c4aec2de98bb5eeb1ed8e84542b0579bdb4 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:15:14 -0600 Subject: [PATCH 17/44] backfill execution_time for all commands batched into a single readOutput --- typescript/__tests__/interactive/session.test.ts | 9 +++++++++ typescript/src/interactive/session.ts | 16 +++++++++++----- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index d1b4dcb..ba0c275 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -622,6 +622,15 @@ describe("InteractiveSession", () => { expect(session.commandHistory[0]!.command).toBe("puts hi"); }); + it("records execution_time for every command batched into one readOutput", async () => { + await session.sendCommand("cmd_a"); + await session.sendCommand("cmd_b"); + await session.readOutput(50); + + expect(session.commandHistory[0]!.execution_time).toBeDefined(); + expect(session.commandHistory[1]!.execution_time).toBeDefined(); + }); + it("getCommandHistory filters by search (case-insensitive)", async () => { await session.sendCommand("report_wns"); await session.sendCommand("get_nets foo"); diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index d14d8a6..7b843e7 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -387,12 +387,18 @@ export class InteractiveSession { return undefined; } - /** Update lastActivity and backfill the last history entry after a read. */ + /** + * Update lastActivity and backfill history entries after a read. Walks back + * over every trailing entry still missing execution_time, stopping at the + * first already-recorded one, so commands batched into a single readOutput + * all get timing instead of only the most recent. + */ private _recordReadResult(outputLength: number, executionTime: number): void { - const last = this.commandHistory[this.commandHistory.length - 1]; - if (last && last.execution_time === undefined) { - last.execution_time = executionTime; - last.output_length = outputLength; + for (let i = this.commandHistory.length - 1; i >= 0; i--) { + const entry = this.commandHistory[i]; + if (!entry || entry.execution_time !== undefined) break; + entry.execution_time = executionTime; + entry.output_length = outputLength; } this.lastActivity = new Date(); } From 0b47c85f7553c841354146fffba01e5de4a8bd26 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:16:32 -0600 Subject: [PATCH 18/44] preserve exit code across cleanup so late waitForExit callers get the real code --- typescript/src/interactive/pty_handler.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index 9d2a356..e66f907 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -135,8 +135,10 @@ export class PtyHandler { } async waitForExit(timeoutMs?: number): Promise { - if (!this._ptyProcess) return null; + // Check the recorded exit code first so callers that arrive after cleanup() + // still get the real code rather than null. if (this._exitCode !== null) return this._exitCode; + if (!this._ptyProcess) return null; return new Promise((resolve) => { let settled = false; @@ -203,6 +205,7 @@ export class PtyHandler { this._alive = false; this._dataDisposable = null; this._exitDisposable = null; - this._exitCode = null; + // Keep _exitCode so a late waitForExit() caller still sees the real exit + // code. createSession() resets it to null when the handler is reused. } } From 4735f8d0a19555f084bfd8e067b7a4e4e0f25454 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:20:18 -0600 Subject: [PATCH 19/44] use a live pid in the pty mock so the kill(0) liveness probe sees an active process --- typescript/__tests__/interactive/pty_handler.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/interactive/pty_handler.test.ts b/typescript/__tests__/interactive/pty_handler.test.ts index b01aabc..1dc4688 100644 --- a/typescript/__tests__/interactive/pty_handler.test.ts +++ b/typescript/__tests__/interactive/pty_handler.test.ts @@ -26,7 +26,9 @@ function makeMockPty(): MockPty { let capturedOnExit: ((e: { exitCode: number; signal?: number }) => void) | undefined; return { - pid: 12345, + // Use the test runner's own pid so isProcessAlive()'s `process.kill(pid, 0)` + // liveness probe sees a real, live process for an unterminated mock. + pid: process.pid, write: vi.fn(), kill: vi.fn(), resize: vi.fn(), From 4b22474c8250cacbe6f1d8dd0edb1f9c6d91b7d1 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:21:06 -0600 Subject: [PATCH 20/44] reject whitespace-only path segments in validatePathSegment --- typescript/__tests__/utils/path_security.test.ts | 6 ++++++ typescript/src/utils/path_security.ts | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/utils/path_security.test.ts b/typescript/__tests__/utils/path_security.test.ts index a013faa..673cabf 100644 --- a/typescript/__tests__/utils/path_security.test.ts +++ b/typescript/__tests__/utils/path_security.test.ts @@ -18,6 +18,12 @@ describe("validatePathSegment", () => { ); }); + it("rejects whitespace-only segment", () => { + expect(() => validatePathSegment(" ", "test_segment")).toThrow( + new ValidationError("test_segment cannot be empty"), + ); + }); + it("rejects '.' segment", () => { expect(() => validatePathSegment(".", "test_segment")).toThrow( new ValidationError("test_segment cannot be '.' or '..'"), diff --git a/typescript/src/utils/path_security.ts b/typescript/src/utils/path_security.ts index 0c57409..2b60008 100644 --- a/typescript/src/utils/path_security.ts +++ b/typescript/src/utils/path_security.ts @@ -3,7 +3,7 @@ import fs from "node:fs"; import { ValidationError } from "../exceptions.js"; export function validatePathSegment(segment: string, segmentName: string): void { - if (!segment) throw new ValidationError(`${segmentName} cannot be empty`); + if (!segment || segment.trim() === "") throw new ValidationError(`${segmentName} cannot be empty`); if (segment === "." || segment === "..") throw new ValidationError(`${segmentName} cannot be '.' or '..'`); if (segment.includes("/") || segment.includes("\\")) throw new ValidationError(`${segmentName} cannot contain path separators`); if (segment.includes("\x00")) throw new ValidationError(`${segmentName} cannot contain null bytes`); From ce3c8964b84720a8a86fbf6f2917f8577e656399 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 07:23:23 -0600 Subject: [PATCH 21/44] bound per-session command history so long-lived sessions do not leak memory --- typescript/__tests__/interactive/session.test.ts | 16 ++++++++++++++++ typescript/src/constants.ts | 4 ++++ typescript/src/interactive/session.ts | 12 +++++++++++- 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index ba0c275..81567a0 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -3,6 +3,7 @@ import { InteractiveSession } from "../../src/interactive/session.js"; import { SessionState } from "../../src/core/models.js"; import { SessionError, SessionTerminatedError } from "../../src/interactive/models.js"; import { Settings } from "../../src/config/settings.js"; +import { MAX_COMMAND_HISTORY } from "../../src/constants.js"; import type { PtyHandler } from "../../src/interactive/pty_handler.js"; vi.mock("node-pty", () => ({ spawn: vi.fn() })); @@ -631,6 +632,21 @@ describe("InteractiveSession", () => { expect(session.commandHistory[1]!.execution_time).toBeDefined(); }); + it("bounds commandHistory at MAX_COMMAND_HISTORY, dropping the oldest", async () => { + // Large queue so rapid sends never hit the input-queue-full guard. + const s = new InteractiveSession("hist-cap", 1024, new Settings({ SESSION_QUEUE_SIZE: 1_000_000 })); + s.pty = makeMockPty(); + await s.start(); + + const total = MAX_COMMAND_HISTORY + 5; + for (let i = 1; i <= total; i++) await s.sendCommand(`c${i}`); + + expect(s.commandHistory).toHaveLength(MAX_COMMAND_HISTORY); + // Oldest entries dropped: first retained command_number is total - MAX + 1. + expect(s.commandHistory[0]!.command_number).toBe(total - MAX_COMMAND_HISTORY + 1); + await s.cleanup(); + }); + it("getCommandHistory filters by search (case-insensitive)", async () => { await session.sendCommand("report_wns"); await session.sendCommand("get_nets foo"); diff --git a/typescript/src/constants.ts b/typescript/src/constants.ts index 2f5e58e..ff27c14 100644 --- a/typescript/src/constants.ts +++ b/typescript/src/constants.ts @@ -24,3 +24,7 @@ export const CHUNK_JOIN_THRESHOLD = 100; export const LARGE_IO_THRESHOLD = 10_000; export const SLOW_OPERATION_THRESHOLD = 1.0; +// Cap on retained per-session command history entries. Bounds memory on +// long-lived sessions; the oldest entries are dropped once the cap is exceeded. +export const MAX_COMMAND_HISTORY = 1000; + diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 7b843e7..c41a663 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -10,7 +10,12 @@ import type { InteractiveSessionInfo, SessionDetailedMetrics, } from "../core/models.js"; -import { BYTES_TO_MB, MAX_COMMAND_COMPLETION_WINDOW, UTILIZATION_PERCENTAGE_BASE } from "../constants.js"; +import { + BYTES_TO_MB, + MAX_COMMAND_COMPLETION_WINDOW, + MAX_COMMAND_HISTORY, + UTILIZATION_PERCENTAGE_BASE, +} from "../constants.js"; import { CircularBuffer } from "./buffer.js"; import { SessionError, SessionTerminatedError } from "./models.js"; import { PtyHandler } from "./pty_handler.js"; @@ -182,6 +187,11 @@ export class InteractiveSession { command_number: this.commandCount + 1, execution_start: Date.now() / 1000, }); + // Bound history so a long-lived session cannot grow it without limit. + // command_number keeps increasing, so dropping the oldest entry is safe. + if (this.commandHistory.length > MAX_COMMAND_HISTORY) { + this.commandHistory.shift(); + } const data = command.endsWith("\n") ? command : command + "\n"; this._inputQueue.push(data); From 56763000aa0135441da03d2f06b06447413e1d48 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 17:48:05 -0600 Subject: [PATCH 22/44] block body-eval builtins and bracket substitution so the query tool cannot be bypassed via catch/if/foreach or set x [exec ls] --- .../config/command_whitelist.test.ts | 46 +++++++++- typescript/src/config/command_whitelist.ts | 92 +++++++++++++++---- 2 files changed, 118 insertions(+), 20 deletions(-) diff --git a/typescript/__tests__/config/command_whitelist.test.ts b/typescript/__tests__/config/command_whitelist.test.ts index 9645767..b3082a9 100644 --- a/typescript/__tests__/config/command_whitelist.test.ts +++ b/typescript/__tests__/config/command_whitelist.test.ts @@ -62,10 +62,16 @@ describe("pattern sets", () => { expect(READONLY_PATTERNS).toContain("check_*"); }); - it("READONLY contains Tcl builtins", () => { + it("READONLY contains safe Tcl builtins", () => { expect(READONLY_PATTERNS).toContain("puts"); - expect(READONLY_PATTERNS).toContain("foreach"); expect(READONLY_PATTERNS).toContain("set"); + expect(READONLY_PATTERNS).toContain("expr"); + }); + + it("READONLY excludes body-eval builtins (if/for/foreach/while/proc/catch/namespace)", () => { + for (const verb of ["if", "for", "foreach", "while", "proc", "catch", "namespace", "uplevel"]) { + expect(READONLY_PATTERNS).not.toContain(verb); + } }); it("EXEC_ONLY contains set_*/read_*/write_* globs", () => { @@ -195,6 +201,42 @@ describe("isQueryCommand", () => { it("splits on semicolons and rejects the offending verb", () => { expect(isQueryCommand("report_wns; global_placement")).toEqual([false, "global_placement"]); }); + + it("body-eval: blocks catch wrapping exec (finding 1)", () => { + expect(isQueryCommand("catch { exec ls }")).toEqual([false, "catch"]); + }); + + it("body-eval: blocks if wrapping exec (finding 1)", () => { + expect(isQueryCommand("if 1 { exec ls }")).toEqual([false, "if"]); + }); + + it("body-eval: blocks foreach wrapping exec (finding 1)", () => { + expect(isQueryCommand("foreach x {a} { exec ls }")).toEqual([false, "foreach"]); + }); + + it("bracket: blocks set x [exec ls] via bracket scan (finding 2)", () => { + expect(isQueryCommand("set x [exec ls]")).toEqual([false, "exec"]); + }); + + it("bracket: blocks expr {[exec ls]} via bracket scan (finding 2)", () => { + expect(isQueryCommand("expr {[exec ls]}")).toEqual([false, "exec"]); + }); + + it("bracket: allows puts [report_wns] when bracket verb is read-only (finding 2)", () => { + expect(isQueryCommand("puts [report_wns]")).toEqual([true, null]); + }); + + it("bracket: blocks puts [global_placement] (exec-only in bracket)", () => { + expect(isQueryCommand("puts [global_placement]")).toEqual([false, "global_placement"]); + }); + + it("semicolon in quoted string is not a statement separator (finding 3)", () => { + expect(isQueryCommand('puts "hello; world"')).toEqual([true, null]); + }); + + it("semicolon inside braces is not a statement separator (finding 3)", () => { + expect(isQueryCommand("report_checks {a; b}")).toEqual([true, null]); + }); }); // isExecCommand diff --git a/typescript/src/config/command_whitelist.ts b/typescript/src/config/command_whitelist.ts index beb6042..028d122 100644 --- a/typescript/src/config/command_whitelist.ts +++ b/typescript/src/config/command_whitelist.ts @@ -76,17 +76,14 @@ export const EXEC_ONLY_PATTERNS: readonly string[] = [ ]; // Safe Tcl built-ins - usable in both tools. +// Intentionally excludes body-eval builtins (if, for, foreach, while, proc, +// catch, namespace, uplevel) because they accept a script body argument and +// can therefore wrap any dangerous command: `catch { exec ls }` would pass the +// verb check on `catch` alone. Those builtins belong in the exec tool. export const _TCL_BUILTINS: readonly string[] = [ "puts", "set", "expr", - "if", - "else", - "elseif", - "for", - "foreach", - "while", - "proc", "return", "break", "continue", @@ -105,9 +102,7 @@ export const _TCL_BUILTINS: readonly string[] = [ "scan", "array", "dict", - "catch", "error", - "namespace", "upvar", "global", "variable", @@ -167,20 +162,81 @@ export function extractVerb(statement: string): string | null { return firstToken.replace(/;+$/, ""); } -/** Iterate the verbs of a command, mirroring Python's naive `;`->newline split. */ +// Unicode line-boundary characters that Python's str.splitlines() recognises. +// Treating every one of these (not just \n) as a statement separator closes a +// \r-based bypass where a second command is hidden after a carriage return. +const STATEMENT_SEPARATORS: ReadonlySet = new Set([ + "\n", + "\r", + "\v", + "\f", + "\x1c", + "\x1d", + "\x1e", + "\x85", + "
", + "
", +]); + +/** + * Split a Tcl command string into individual statements, respecting quoted + * strings and brace groups so that separators inside them are not treated as + * statement boundaries (e.g. `puts "hello; world"` is one statement). Statement + * separators are `;` plus every Unicode line boundary in STATEMENT_SEPARATORS, + * so a bare \r cannot hide a second command from the verb check. + */ +function splitTclStatements(command: string): string[] { + const stmts: string[] = []; + let depth = 0; + let inQuote = false; + let current = ""; + + for (let i = 0; i < command.length; i++) { + const ch = command[i]!; + if (ch === "\\" && i + 1 < command.length) { + current += ch + command[i + 1]!; + i++; + } else if (ch === '"' && depth === 0) { + inQuote = !inQuote; + current += ch; + } else if (!inQuote && ch === "{") { + depth++; + current += ch; + } else if (!inQuote && ch === "}") { + depth--; + current += ch; + } else if (!inQuote && depth === 0 && (ch === ";" || STATEMENT_SEPARATORS.has(ch))) { + stmts.push(current); + current = ""; + } else { + current += ch; + } + } + if (current) stmts.push(current); + return stmts; +} + +/** + * Iterate the verbs of a Tcl command string. + * + * Two passes: + * 1. Statement verbs — the leading token of each statement. The split is + * Tcl-aware (skips separators inside quotes/braces) and treats `;` plus all + * Unicode line boundaries as separators so a \r cannot hide a command. + * 2. Bracket verbs — the word immediately following each `[` (bracket + * substitution). This catches `set x [exec ls]` where the outer verb `set` + * is safe but the substituted command `exec` is not. + */ function* iterVerbs(command: string): Generator { - // Replace ';' with newline, then split on all Unicode line boundaries that - // Python's str.splitlines() recognises. \r\n must precede \r so the pair is - // consumed as one separator. Semicolons inside Tcl braces or quoted strings - // are not handled - this matches the Python implementation's known limitation. - for (const rawLine of command - .replace(/;/g, "\n") - .split(/\r\n|[\n\r\v\f\x1c\x1d\x1e\x85\u2028\u2029]/)) { - const verb = extractVerb(rawLine); + for (const stmt of splitTclStatements(command)) { + const verb = extractVerb(stmt); if (verb !== null) { yield verb; } } + for (const match of command.matchAll(/\[(\w+)/g)) { + yield match[1]!; + } } /** From 6df597fcbc1277b3ef3505ae5eac397f4316e0ae Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 17:53:36 -0600 Subject: [PATCH 23/44] terminate auto-created session when executeCommand throws so the session is not leaked --- typescript/src/tools/interactive.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/typescript/src/tools/interactive.ts b/typescript/src/tools/interactive.ts index 8433ced..7badb18 100644 --- a/typescript/src/tools/interactive.ts +++ b/typescript/src/tools/interactive.ts @@ -146,6 +146,11 @@ export class QueryShellTool extends BaseTool { ); return this.formatResult(result as unknown as Record); } catch (e) { + // Auto-created sessions must be cleaned up when executeCommand throws to + // avoid leaking the session. + if (sid === null && resolvedId !== null) { + this.manager.terminateSession(resolvedId, true).catch(() => { /* best effort */ }); + } if (e instanceof SessionNotFoundError) { return this.formatResult( sessionNotFoundExecResult( @@ -200,6 +205,9 @@ export class ExecShellTool extends BaseTool { ); return this.formatResult(result as unknown as Record); } catch (e) { + if (sid === null && resolvedId !== null) { + this.manager.terminateSession(resolvedId, true).catch(() => { /* best effort */ }); + } if (e instanceof SessionNotFoundError) { return this.formatResult( sessionNotFoundExecResult( From d5b499a276c259acf8ac8f402cd38fc1494b038a Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 17:54:25 -0600 Subject: [PATCH 24/44] derive wasAlive from info.isAlive and propagate it through error branches in TerminateSessionTool --- typescript/src/tools/interactive.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/typescript/src/tools/interactive.ts b/typescript/src/tools/interactive.ts index 7badb18..ba0cc99 100644 --- a/typescript/src/tools/interactive.ts +++ b/typescript/src/tools/interactive.ts @@ -306,13 +306,12 @@ export class TerminateSessionTool extends BaseTool { } async execute(sessionId: string, force = false): Promise { - let wasAlive = true; + let wasAlive = false; try { - await this.manager.getSessionInfo(sessionId); + const info = await this.manager.getSessionInfo(sessionId); + wasAlive = info.isAlive; } catch (e) { - if (e instanceof SessionNotFoundError) { - wasAlive = false; - } + if (!(e instanceof SessionNotFoundError)) throw e; } try { @@ -331,6 +330,7 @@ export class TerminateSessionTool extends BaseTool { SessionTerminationResult.parse({ sessionId, terminated: false, + wasAlive, error: String(e), }) as unknown as Record, ); @@ -339,6 +339,7 @@ export class TerminateSessionTool extends BaseTool { SessionTerminationResult.parse({ sessionId, terminated: false, + wasAlive, error: `Termination failed: ${(e as Error).message ?? String(e)}`, }) as unknown as Record, ); From c132034b0cd2264f06559002035779401bce70ba Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 17:58:53 -0600 Subject: [PATCH 25/44] discard queued commands and warn on terminate so pending inputs are not silently dropped --- typescript/src/interactive/session.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index c41a663..a5d0696 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -1,6 +1,7 @@ import pidusage from "pidusage"; import { Mutex } from "async-mutex"; import { ANSIDecoder } from "../utils/ansi_decoder.js"; +import { getLogger } from "../utils/logging.js"; import { getSettings } from "../config/settings.js"; import type { Settings } from "../config/settings.js"; import { SessionState } from "../core/models.js"; @@ -293,6 +294,12 @@ export class InteractiveSession { await this._lifecycleLock.runExclusive(async () => { if (this._state === SessionState.TERMINATED) return; + if (this._inputQueue.length > 0) { + getLogger("session").warn( + `Session ${this.sessionId}: discarding ${this._inputQueue.length} pending command(s) on terminate`, + ); + this._inputQueue.length = 0; + } this.state = SessionState.TERMINATED; this._signalShutdown(); From 07e423a5f888dbf8cc83584209028d690d33b245 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 18:00:00 -0600 Subject: [PATCH 26/44] rename isAlive to checkAlive on InteractiveSession to surface its state-transition side effect --- typescript/__tests__/core/manager.test.ts | 4 ++-- typescript/__tests__/interactive/session.test.ts | 16 ++++++++-------- typescript/src/core/manager.ts | 4 ++-- typescript/src/interactive/session.ts | 16 +++++++++++----- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/typescript/__tests__/core/manager.test.ts b/typescript/__tests__/core/manager.test.ts index e8e6338..976356f 100644 --- a/typescript/__tests__/core/manager.test.ts +++ b/typescript/__tests__/core/manager.test.ts @@ -16,7 +16,7 @@ vi.mock("../../src/interactive/session.js", () => { interface MockSession { sessionId: string; lastActivity: Date; - isAlive: Mock; + checkAlive: Mock; start: Mock; sendCommand: Mock; readOutput: Mock; @@ -47,7 +47,7 @@ function makeMockSession(sessionId: string, alive = true): MockSession { return { sessionId, lastActivity: new Date(), - isAlive: vi.fn().mockReturnValue(alive), + checkAlive: vi.fn().mockReturnValue(alive), start: vi.fn().mockResolvedValue(undefined), sendCommand: vi.fn().mockResolvedValue(undefined), readOutput: vi.fn().mockResolvedValue({ diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index 81567a0..e8acb83 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -40,7 +40,7 @@ describe("InteractiveSession", () => { expect(session.sessionId).toBe("test-session-1"); expect(session.state).toBe(SessionState.CREATING); expect(session.commandCount).toBe(0); - expect(session.isAlive()).toBe(false); + expect(session.checkAlive()).toBe(false); expect(session.pty).not.toBeNull(); expect(session.outputBuffer).not.toBeNull(); }); @@ -256,17 +256,17 @@ describe("InteractiveSession", () => { }); }); - describe("isAlive", () => { + describe("checkAlive", () => { it("returns false in CREATING state", () => { expect(session.state).toBe(SessionState.CREATING); - expect(session.isAlive()).toBe(false); + expect(session.checkAlive()).toBe(false); }); it("returns false in ACTIVE state when process is dead", () => { session.state = SessionState.ACTIVE; (mockPty.isProcessAlive as ReturnType).mockReturnValue(false); - expect(session.isAlive()).toBe(false); + expect(session.checkAlive()).toBe(false); expect(session.state).toBe(SessionState.TERMINATED); }); @@ -287,13 +287,13 @@ describe("InteractiveSession", () => { session.state = SessionState.ACTIVE; (mockPty.isProcessAlive as ReturnType).mockReturnValue(true); - expect(session.isAlive()).toBe(true); + expect(session.checkAlive()).toBe(true); expect(session.state).toBe(SessionState.ACTIVE); }); it("returns false in TERMINATED state", () => { session.state = SessionState.TERMINATED; - expect(session.isAlive()).toBe(false); + expect(session.checkAlive()).toBe(false); }); }); @@ -437,7 +437,7 @@ describe("InteractiveSession", () => { await new Promise((r) => setTimeout(r, 5)); expect(session.state).toBe(SessionState.TERMINATED); - expect(session.isAlive()).toBe(false); + expect(session.checkAlive()).toBe(false); }); it("onData data exactly at READ_CHUNK_SIZE is a single append, not sliced", async () => { @@ -542,7 +542,7 @@ describe("InteractiveSession", () => { expect(mockPty.writeInput).toHaveBeenCalled(); expect(session.state).toBe(SessionState.TERMINATED); - expect(session.isAlive()).toBe(false); + expect(session.checkAlive()).toBe(false); }); it("subsequent sendCommand throws SessionTerminatedError after writer failure", async () => { diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index f9325d7..472d9e6 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -259,7 +259,7 @@ export class OpenROADManager { private _countActive(): number { let count = 0; for (const session of this.sessions.values()) { - if (session !== null && session.isAlive()) count++; + if (session !== null && session.checkAlive()) count++; } return count; } @@ -292,7 +292,7 @@ export class OpenROADManager { const terminated: Array<[string, InteractiveSession, boolean]> = []; for (const [sessionId, session] of this._initializedSessions()) { - if (!session.isAlive()) { + if (!session.checkAlive()) { // Measure from the actual death time. lastActivity is the last command, // which for a long-idle session is far earlier and would trip the // force-cleanup timer immediately. Fall back to lastActivity only if the diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index a5d0696..63c698a 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -94,7 +94,13 @@ export class InteractiveSession { return this._terminatedAt; } - isAlive(): boolean { + /** + * Check whether the session is alive, syncing state as a side effect. + * If the underlying PTY process has died since the last check, this + * transitions _state to TERMINATED and signals the writer to stop. + * Named checkAlive (not isAlive) to signal that it is not a pure predicate. + */ + checkAlive(): boolean { if (this._state === SessionState.TERMINATED) return false; const processAlive = this.pty.isProcessAlive(); @@ -169,7 +175,7 @@ export class InteractiveSession { } async sendCommand(command: string): Promise { - if (!this.isAlive()) { + if (!this.checkAlive()) { throw new SessionTerminatedError(`Session ${this.sessionId} is not active`, this.sessionId); } @@ -207,7 +213,7 @@ export class InteractiveSession { async readOutput(timeoutMs = 1000): Promise { const startTime = Date.now(); - if (!this.isAlive()) { + if (!this.checkAlive()) { // Drain-before-reject: a fast-exiting command (e.g. "exit") can flip // _state to TERMINATED between sendCommand and readOutput because // sendCommand is synchronous and the event loop runs onExit at the @@ -282,7 +288,7 @@ export class InteractiveSession { return { sessionId: this.sessionId, createdAt: this.createdAt.toISOString(), - isAlive: this.isAlive(), + isAlive: this.checkAlive(), commandCount: this.commandCount, bufferSize: this.outputBuffer.size, uptimeSeconds: uptime, @@ -456,7 +462,7 @@ export class InteractiveSession { return { session_id: this.sessionId, state: this._state, - is_alive: this.isAlive(), + is_alive: this.checkAlive(), created_at: this.createdAt.toISOString(), last_activity: this.lastActivity.toISOString(), uptime_seconds: uptimeSeconds, From 69df939ca537bd67f5d3b814152183c047d72170 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 18:01:09 -0600 Subject: [PATCH 27/44] wrap non-force cleanup in finally so a throwing cleanup still removes the dead session from the map --- typescript/src/core/manager.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index 472d9e6..36a67ad 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -317,9 +317,12 @@ export class OpenROADManager { cleaned++; } } else { - await session.cleanup(); - this.sessions.delete(sessionId); - cleaned++; + try { + await session.cleanup(); + } finally { + this.sessions.delete(sessionId); + cleaned++; + } } } catch (e) { this.logger.error(`Error during session ${sessionId} cleanup: ${String(e)}`); From 569bde7a25b8b56d23445d4b361a910d4857e195 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 18:01:16 -0600 Subject: [PATCH 28/44] snapshot session counts after the async metrics loop so the totals reflect post-loop state --- typescript/src/core/manager.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index 36a67ad..f899fe1 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -172,10 +172,6 @@ export class OpenROADManager { async sessionMetrics(): Promise { await this._cleanupTerminatedSessionsWithLock(); - const totalSessions = this.sessions.size; - const activeSessions = this.getActiveSessionCount(); - const terminatedSessions = totalSessions - activeSessions; - const sessionDetails: SessionDetailedMetrics[] = []; let totalCommands = 0; let totalCpuTime = 0; @@ -193,6 +189,12 @@ export class OpenROADManager { } } + // Capture counts after the async loop so the snapshot reflects the final + // state rather than a stale pre-loop value. + const totalSessions = this.sessions.size; + const activeSessions = this.getActiveSessionCount(); + const terminatedSessions = totalSessions - activeSessions; + return { manager: { total_sessions: totalSessions, From 47e8e7768c6c7382cc339f19aeb4f15174aa6cde Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 18:01:23 -0600 Subject: [PATCH 29/44] remove dead session loop in cleanupAll since terminateAllSessions already empties the map --- typescript/src/core/manager.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index f899fe1..a84c539 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -231,20 +231,7 @@ export class OpenROADManager { async cleanupAll(): Promise { this.logger.info("Starting OpenROAD cleanup"); - await this.terminateAllSessions(true); - - await this.cleanupLock.runExclusive(async () => { - for (const [, session] of this._initializedSessions()) { - try { - await session.cleanup(); - } catch (e) { - this.logger.warn(`Error during session cleanup: ${String(e)}`); - } - } - this.sessions.clear(); - }); - this.logger.info("OpenROAD cleanup completed"); } From 43605622d227f65356ae55c5a5271f2b524098b5 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 18:53:45 -0600 Subject: [PATCH 30/44] call terminateProcess with force=true in cleanup so a stuck process is not retried with SIGTERM --- typescript/src/interactive/pty_handler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index e66f907..ade87b2 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -189,7 +189,7 @@ export class PtyHandler { async cleanup(): Promise { if (this._alive) { try { - await this.terminateProcess(); + await this.terminateProcess(true); } catch { // Best effort - don't let terminate errors prevent state reset } From b1475736b4dcd71b59f62259859cbb0ba5b905ab Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 18:54:00 -0600 Subject: [PATCH 31/44] guard against null image dimensions before resize so missing metadata does not produce a silent 256x256 output --- typescript/src/tools/report_images.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/typescript/src/tools/report_images.ts b/typescript/src/tools/report_images.ts index 8daf037..2be681b 100644 --- a/typescript/src/tools/report_images.ts +++ b/typescript/src/tools/report_images.ts @@ -191,8 +191,11 @@ async function loadAndCompressImage( const targetBytes = Math.floor((maxSizeKb * 1024 * 3) / 4); const scale = Math.sqrt(targetBytes / originalSize); const meta = await sharp(imagePath).metadata(); - const origW = meta.width ?? 0; - const origH = meta.height ?? 0; + if (!meta.width || !meta.height) { + throw new Error("Image dimensions unavailable"); + } + const origW = meta.width; + const origH = meta.height; const newW = Math.max(Math.round(origW * scale), 256); const newH = Math.max(Math.round(origH * scale), 256); const compressed = await sharp(imagePath) From a1d14ae50b95b6c9d6347a4f15b6494a686ad9a5 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 19:23:50 -0600 Subject: [PATCH 32/44] update integration_check to call checkAlive instead of the renamed isAlive --- typescript/scripts/integration_check.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/typescript/scripts/integration_check.ts b/typescript/scripts/integration_check.ts index ce61330..96a06e9 100644 --- a/typescript/scripts/integration_check.ts +++ b/typescript/scripts/integration_check.ts @@ -38,7 +38,7 @@ async function run() { await session.start(["openroad", "-no_init"]); check("start() succeeds", true); check("state is ACTIVE after start", session.state === "active", session.state); - check("isAlive() returns true", session.isAlive()); + check("isAlive() returns true", session.checkAlive()); check("writer task running", session.isRunning()); } catch (e) { check("start() succeeds", false, String(e)); @@ -98,7 +98,7 @@ async function run() { await session.cleanup(); check("cleanup() does not throw", true); check("state is TERMINATED after cleanup", session.state === "terminated", session.state); - check("isAlive() returns false after cleanup", !session.isAlive()); + check("isAlive() returns false after cleanup", !session.checkAlive()); // ── Summary ───────────────────────────────────────────────────────────────── const passed = results.filter((r) => r.ok).length; From cdddda4131a3d157dc500238d56b1e2cf43605ed Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 19:23:57 -0600 Subject: [PATCH 33/44] extend bracket-scan regex to match [::exec ls] so namespace-qualified commands are not exempt --- typescript/__tests__/config/command_whitelist.test.ts | 4 ++++ typescript/src/config/command_whitelist.ts | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/config/command_whitelist.test.ts b/typescript/__tests__/config/command_whitelist.test.ts index b3082a9..0384a66 100644 --- a/typescript/__tests__/config/command_whitelist.test.ts +++ b/typescript/__tests__/config/command_whitelist.test.ts @@ -218,6 +218,10 @@ describe("isQueryCommand", () => { expect(isQueryCommand("set x [exec ls]")).toEqual([false, "exec"]); }); + it("bracket: blocks set x [::exec ls] with namespace-qualified command", () => { + expect(isQueryCommand("set x [::exec ls]")).toEqual([false, "exec"]); + }); + it("bracket: blocks expr {[exec ls]} via bracket scan (finding 2)", () => { expect(isQueryCommand("expr {[exec ls]}")).toEqual([false, "exec"]); }); diff --git a/typescript/src/config/command_whitelist.ts b/typescript/src/config/command_whitelist.ts index 028d122..a940d1f 100644 --- a/typescript/src/config/command_whitelist.ts +++ b/typescript/src/config/command_whitelist.ts @@ -234,7 +234,7 @@ function* iterVerbs(command: string): Generator { yield verb; } } - for (const match of command.matchAll(/\[(\w+)/g)) { + for (const match of command.matchAll(/\[\s*(?::+)?(\w+)/g)) { yield match[1]!; } } From 587952c6f4eb0936a73be596ad1294927c3f237e Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 19:52:43 -0600 Subject: [PATCH 34/44] code cleanup --- .../__tests__/interactive/buffer.test.ts | 8 +-- .../__tests__/interactive/session.test.ts | 13 ++--- .../__tests__/tools/interactive.test.ts | 45 +--------------- .../__tests__/tools/report_images.test.ts | 52 +++---------------- typescript/scripts/integration_check.ts | 25 +++------ typescript/src/config/settings.ts | 11 ---- typescript/src/constants.ts | 14 ++--- typescript/src/core/manager.ts | 41 ++++++--------- typescript/src/interactive/buffer.ts | 10 ++-- typescript/src/interactive/pty_handler.ts | 19 +++---- typescript/src/main.ts | 5 +- typescript/src/tools/base.ts | 15 +++--- typescript/src/tools/interactive.ts | 35 ++----------- typescript/src/tools/report_images.ts | 44 +++------------- typescript/src/utils/ansi_decoder.ts | 30 ++++------- typescript/src/utils/logging.ts | 21 ++------ typescript/src/utils/path_security.ts | 8 +-- 17 files changed, 93 insertions(+), 303 deletions(-) diff --git a/typescript/__tests__/interactive/buffer.test.ts b/typescript/__tests__/interactive/buffer.test.ts index b3ca8fa..ea0a4d6 100644 --- a/typescript/__tests__/interactive/buffer.test.ts +++ b/typescript/__tests__/interactive/buffer.test.ts @@ -143,9 +143,9 @@ describe("CircularBuffer", () => { // Start waitForData (fast-path sees _dataAvailable = false and enters the Promise) const waiter = buf.waitForData(5000); - // append() fires here — before runExclusive's callback has a chance to push - // wakeUp into _resolvers. Without the re-check inside runExclusive, wakeUp - // would be pushed after append() already drained an empty _resolvers, and + // append() fires before runExclusive's callback can push wakeUp into + // _resolvers. Without the re-check inside runExclusive, wakeUp would + // land in _resolvers after append() already drained an empty list, and // the caller would wait the full 5-second timeout. await buf.append("raced data"); @@ -168,7 +168,7 @@ describe("CircularBuffer", () => { it("wakes pending waitForData() immediately with false so callers do not hang", async () => { const buf = new CircularBuffer(100); - // waitForData with a large timeout — clear() must unblock it before the timeout fires + // Large timeout: clear() must unblock waitForData before it fires. const waiter = buf.waitForData(5000); await buf.clear(); diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index e8acb83..ce6a575 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -217,7 +217,6 @@ describe("InteractiveSession", () => { await session.outputBuffer.append("% Exiting OpenROAD\r\n"); session.state = SessionState.TERMINATED; - // Must NOT throw even though the session is terminated const result = await session.readOutput(100); expect(result.output).toContain("Exiting OpenROAD"); @@ -227,8 +226,8 @@ describe("InteractiveSession", () => { it("signals shutdown when readOutput detects terminated session so writer task does not loop indefinitely", async () => { // Spy on the private method to verify readOutput() calls it directly. - // Scenario: _state was flipped to TERMINATED externally (e.g. via the setter) - // without calling _signalShutdown() — the exact gap @luarss identified. + // Scenario: _state was flipped to TERMINATED externally (e.g. via the + // setter) without calling _signalShutdown(). const signalShutdown = vi.spyOn(session as unknown as { _signalShutdown: () => void }, "_signalShutdown"); session.state = SessionState.TERMINATED; @@ -327,12 +326,11 @@ describe("InteractiveSession", () => { it("calls pty.cleanup() so listeners and pending resolvers are disposed without a subsequent session.cleanup()", async () => { session.state = SessionState.ACTIVE; - // terminate() without any follow-up cleanup() call await session.terminate(false); - // pty.cleanup() must have been called to dispose _dataDisposable, - // _exitDisposable, and drain _exitResolvers — otherwise post-kill - // data bursts keep appending and waitForExit() callers hang forever + // pty.cleanup() must run to dispose _dataDisposable, _exitDisposable, + // and drain _exitResolvers; otherwise post-kill data bursts keep + // appending and waitForExit() callers hang forever. expect(mockPty.cleanup).toHaveBeenCalledOnce(); }); }); @@ -420,7 +418,6 @@ describe("InteractiveSession", () => { await session.start(["echo"]); session.state = SessionState.TERMINATED; - // Should not throw or double-signal shutdown capturedOnExit?.(0); expect(session.state).toBe(SessionState.TERMINATED); }); diff --git a/typescript/__tests__/tools/interactive.test.ts b/typescript/__tests__/tools/interactive.test.ts index bf1ef74..4db43df 100644 --- a/typescript/__tests__/tools/interactive.test.ts +++ b/typescript/__tests__/tools/interactive.test.ts @@ -6,10 +6,6 @@ import { SessionNotFoundError, SessionTerminatedError, SessionError } from "../. import { SessionState } from "../../src/core/models.js"; import type { InteractiveExecResult, InteractiveSessionInfo, SessionDetailedMetrics, ManagerMetrics } from "../../src/core/models.js"; -// --------------------------------------------------------------------------- -// Mock helpers -// --------------------------------------------------------------------------- - const NOW = "2024-01-01T00:00:00.000Z"; function makeExecResult(overrides: Partial = {}): InteractiveExecResult { @@ -87,10 +83,6 @@ function makeMockManager(): MockManager { }; } -// --------------------------------------------------------------------------- -// QueryShellTool -// --------------------------------------------------------------------------- - describe("QueryShellTool", () => { let mgr: MockManager; let tool: QueryShellTool; @@ -149,7 +141,6 @@ describe("QueryShellTool", () => { }); it("blocks dangerous commands when whitelist is enabled", async () => { - // `quit` is in BLOCKED_COMMANDS const raw = await tool.execute("quit"); const result = JSON.parse(raw); expect(result.error).toMatch(/CommandBlocked/); @@ -161,10 +152,6 @@ describe("QueryShellTool", () => { }); }); -// --------------------------------------------------------------------------- -// ExecShellTool -// --------------------------------------------------------------------------- - describe("ExecShellTool", () => { let mgr: MockManager; let tool: ExecShellTool; @@ -195,10 +182,6 @@ describe("ExecShellTool", () => { }); }); -// --------------------------------------------------------------------------- -// ListSessionsTool -// --------------------------------------------------------------------------- - describe("ListSessionsTool", () => { let mgr: MockManager; let tool: ListSessionsTool; @@ -238,10 +221,6 @@ describe("ListSessionsTool", () => { }); }); -// --------------------------------------------------------------------------- -// CreateSessionTool -// --------------------------------------------------------------------------- - describe("CreateSessionTool", () => { let mgr: MockManager; let tool: CreateSessionTool; @@ -278,10 +257,6 @@ describe("CreateSessionTool", () => { }); }); -// --------------------------------------------------------------------------- -// TerminateSessionTool -// --------------------------------------------------------------------------- - describe("TerminateSessionTool", () => { let mgr: MockManager; let tool: TerminateSessionTool; @@ -325,10 +300,6 @@ describe("TerminateSessionTool", () => { }); }); -// --------------------------------------------------------------------------- -// InspectSessionTool -// --------------------------------------------------------------------------- - describe("InspectSessionTool", () => { let mgr: MockManager; let tool: InspectSessionTool; @@ -363,10 +334,6 @@ describe("InspectSessionTool", () => { }); }); -// --------------------------------------------------------------------------- -// SessionHistoryTool -// --------------------------------------------------------------------------- - describe("SessionHistoryTool", () => { let mgr: MockManager; let tool: SessionHistoryTool; @@ -410,10 +377,6 @@ describe("SessionHistoryTool", () => { }); }); -// --------------------------------------------------------------------------- -// SessionMetricsTool -// --------------------------------------------------------------------------- - describe("SessionMetricsTool", () => { let mgr: MockManager; let tool: SessionMetricsTool; @@ -440,12 +403,8 @@ describe("SessionMetricsTool", () => { }); }); -// --------------------------------------------------------------------------- -// Integration: full workflow -// --------------------------------------------------------------------------- - describe("Integration: session workflow", () => { - it("create → execute → list → terminate", async () => { + it("create, execute, list, terminate", async () => { const mgr = makeMockManager(); mgr.listSessions.mockResolvedValue([makeSessionInfo()]); @@ -476,9 +435,7 @@ describe("Integration: session workflow", () => { }); }); -// --------------------------------------------------------------------------- // Snapshot: one representative output per tool -// --------------------------------------------------------------------------- describe("Snapshots: wire format stability", () => { it("QueryShellTool success output", async () => { diff --git a/typescript/__tests__/tools/report_images.test.ts b/typescript/__tests__/tools/report_images.test.ts index 1485588..44a41c6 100644 --- a/typescript/__tests__/tools/report_images.test.ts +++ b/typescript/__tests__/tools/report_images.test.ts @@ -10,10 +10,7 @@ import { } from "../../src/tools/report_images.js"; import type { OpenROADManager } from "../../src/core/manager.js"; -// --------------------------------------------------------------------------- -// Mock getSettings so tests don't depend on the filesystem ORFS installation -// --------------------------------------------------------------------------- - +// Mock getSettings so tests do not depend on a filesystem ORFS install. vi.mock("../../src/config/settings.js", () => { let mockFlowPath = "/mock/flow"; let mockPlatforms: string[] = []; @@ -47,10 +44,6 @@ vi.mock("../../src/config/settings.js", () => { import { getSettings } from "../../src/config/settings.js"; -// --------------------------------------------------------------------------- -// Fixture helpers -// --------------------------------------------------------------------------- - let tmpDir: string; function createFixture( @@ -60,10 +53,8 @@ function createFixture( imageFiles: string[] = ["cts_clk.webp", "final_all.webp"], ) { const flowPath = tmpDir; - // Settings directories (used by platforms / designs accessors) fs.mkdirSync(path.join(flowPath, "platforms", platform), { recursive: true }); fs.mkdirSync(path.join(flowPath, "designs", platform, design), { recursive: true }); - // Reports directory const runPath = path.join(flowPath, "reports", platform, design, runSlug); fs.mkdirSync(runPath, { recursive: true }); for (const img of imageFiles) { @@ -72,11 +63,9 @@ function createFixture( return { flowPath, runPath }; } -// Stub manager (tools don't call manager methods directly, but constructor requires it) +// Constructor requires a manager but the tools never invoke it. const stubManager = {} as unknown as OpenROADManager; -// --------------------------------------------------------------------------- - beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openroad-test-")); }); @@ -86,10 +75,6 @@ afterEach(() => { vi.clearAllMocks(); }); -// --------------------------------------------------------------------------- -// classifyImageType -// --------------------------------------------------------------------------- - describe("classifyImageType", () => { it("classifies CTS images correctly", () => { expect(classifyImageType("cts_clk.webp")).toEqual(["cts", "clock_visualization"]); @@ -114,10 +99,6 @@ describe("classifyImageType", () => { }); }); -// --------------------------------------------------------------------------- -// validatePlatformDesign -// --------------------------------------------------------------------------- - describe("validatePlatformDesign", () => { it("throws on unknown platform", () => { (getSettings as ReturnType).mockReturnValueOnce({ @@ -138,10 +119,6 @@ describe("validatePlatformDesign", () => { }); }); -// --------------------------------------------------------------------------- -// ListReportImagesTool -// --------------------------------------------------------------------------- - describe("ListReportImagesTool", () => { let tool: ListReportImagesTool; @@ -174,7 +151,7 @@ describe("ListReportImagesTool", () => { }); it("returns totalImages 0 when run directory has no .webp files", async () => { - const { flowPath, runPath } = createFixture("nangate45", "gcd", "run-empty", []); + const { flowPath } = createFixture("nangate45", "gcd", "run-empty", []); (getSettings as ReturnType).mockReturnValue({ platforms: ["nangate45"], designs: (p: string) => (p === "nangate45" ? ["gcd"] : []), @@ -222,10 +199,6 @@ describe("ListReportImagesTool", () => { }); }); -// --------------------------------------------------------------------------- -// ReadReportImageTool -// --------------------------------------------------------------------------- - describe("ReadReportImageTool", () => { let tool: ReadReportImageTool; @@ -281,13 +254,10 @@ describe("ReadReportImageTool", () => { }); const raw = await tool.execute("nangate45", "gcd", "run-123", "cts_clk.webp"); const result = JSON.parse(raw); - // Should have image_data as a base64 string expect(typeof result.image_data).toBe("string"); expect(result.image_data.length).toBeGreaterThan(0); - // Round-trip check const decoded = Buffer.from(result.image_data, "base64"); expect(decoded.length).toBeGreaterThan(0); - // Metadata presence expect(result.metadata).toBeTruthy(); expect(result.metadata.filename).toBe("cts_clk.webp"); expect(result.metadata.stage).toBe("cts"); @@ -296,7 +266,6 @@ describe("ReadReportImageTool", () => { it("returns FileTooLarge error when image exceeds 50 MB", async () => { const { flowPath, runPath } = createFixture("nangate45", "gcd", "run-123", []); - // Write a "file" that appears to be 51 MB by mocking statSync const bigPath = path.join(runPath, "huge.webp"); fs.writeFileSync(bigPath, Buffer.from("tiny content")); (getSettings as ReturnType).mockReturnValue({ @@ -330,10 +299,6 @@ describe("ReadReportImageTool", () => { }); }); -// --------------------------------------------------------------------------- -// TestPathTraversalSecurity -// --------------------------------------------------------------------------- - describe("TestPathTraversalSecurity", () => { let tool: ListReportImagesTool; let readTool: ReadReportImageTool; @@ -390,27 +355,22 @@ describe("TestPathTraversalSecurity", () => { }); it("blocks symlink escape from run directory", async () => { - // Create a symlink inside run-123 that points outside const runPath = path.join(flowPath, "reports", "nangate45", "gcd", "run-123"); const linkPath = path.join(runPath, "escape.webp"); try { fs.symlinkSync("/etc/passwd", linkPath); } catch { - // symlink creation may fail in some environments — skip gracefully + // symlink creation may fail in some environments; skip gracefully. return; } const raw = await readTool.execute("nangate45", "gcd", "run-123", "escape.webp"); const result = JSON.parse(raw); - // Should either not find the image, reject path containment, or return an error - // — but must NOT return valid image_data that resolves to /etc/passwd content + // Should not find the image, reject path containment, or return an error + // and must NOT return valid image_data resolving to /etc/passwd content. expect(result.image_data === null || result.error !== null).toBe(true); }); }); -// --------------------------------------------------------------------------- -// TestPlatformDesignValidationInTools -// --------------------------------------------------------------------------- - describe("TestPlatformDesignValidationInTools", () => { beforeEach(() => { (getSettings as ReturnType).mockReturnValue({ diff --git a/typescript/scripts/integration_check.ts b/typescript/scripts/integration_check.ts index 96a06e9..d162fb1 100644 --- a/typescript/scripts/integration_check.ts +++ b/typescript/scripts/integration_check.ts @@ -1,18 +1,13 @@ -/** - * Real OpenROAD REPL integration check. - * Run with: npx tsx scripts/integration_check.ts - */ - import { InteractiveSession } from "../src/interactive/session.js"; import { Settings } from "../src/config/settings.js"; -const PASS = "✓"; -const FAIL = "✗"; +const PASS = "PASS"; +const FAIL = "FAIL"; const results: { label: string; ok: boolean; detail?: string }[] = []; function check(label: string, ok: boolean, detail?: string) { results.push({ label, ok, detail }); - console.log(` ${ok ? PASS : FAIL} ${label}${detail ? ` → ${detail}` : ""}`); + console.log(` ${ok ? PASS : FAIL} ${label}${detail ? ` -> ${detail}` : ""}`); } async function waitForPrompt(session: InteractiveSession, timeoutMs = 5000): Promise { @@ -32,20 +27,18 @@ async function run() { const settings = new Settings({ ENABLE_COMMAND_VALIDATION: false }); const session = new InteractiveSession("integration-check", 256 * 1024, settings); - // ── 1. Spawn ──────────────────────────────────────────────────────────────── console.log("1. Session lifecycle"); try { await session.start(["openroad", "-no_init"]); check("start() succeeds", true); check("state is ACTIVE after start", session.state === "active", session.state); - check("isAlive() returns true", session.checkAlive()); + check("checkAlive() returns true", session.checkAlive()); check("writer task running", session.isRunning()); } catch (e) { check("start() succeeds", false, String(e)); process.exit(1); } - // ── 2. Initial prompt ─────────────────────────────────────────────────────── console.log("\n2. Initial prompt"); const banner = await waitForPrompt(session, 6000); check("received output after spawn", banner.length > 0, `${banner.length} chars`); @@ -55,7 +48,6 @@ async function run() { banner.slice(0, 80).replace(/\n/g, " "), ); - // ── 3. puts echo ──────────────────────────────────────────────────────────── console.log("\n3. Command round-trip"); await session.sendCommand('puts "hello_integration"'); const echoResult = await session.readOutput(3000); @@ -67,7 +59,6 @@ async function run() { ); check("commandCount incremented", session.commandCount >= 1, String(session.commandCount)); - // ── 4. Error detection ────────────────────────────────────────────────────── console.log("\n4. Error detection"); await session.sendCommand("nonexistent_command_xyz"); const errResult = await session.readOutput(3000); @@ -77,7 +68,6 @@ async function run() { errResult.error ?? "(null)", ); - // ── 5. Multiple commands ──────────────────────────────────────────────────── console.log("\n5. Multiple sequential commands"); const before = session.commandCount; await session.sendCommand('puts "cmd1"'); @@ -86,24 +76,21 @@ async function run() { await session.readOutput(1000); check("commandCount advances correctly", session.commandCount === before + 2, String(session.commandCount)); - // ── 6. Buffer ─────────────────────────────────────────────────────────────── console.log("\n6. Output buffer"); const stats = await session.outputBuffer.getStats(); check("buffer maxSize is set", stats.maxSize > 0, `${stats.maxSize} chars`); - // ── 7. Graceful termination ───────────────────────────────────────────────── console.log("\n7. Termination"); await session.sendCommand("exit"); await new Promise((r) => setTimeout(r, 500)); await session.cleanup(); check("cleanup() does not throw", true); check("state is TERMINATED after cleanup", session.state === "terminated", session.state); - check("isAlive() returns false after cleanup", !session.checkAlive()); + check("checkAlive() returns false after cleanup", !session.checkAlive()); - // ── Summary ───────────────────────────────────────────────────────────────── const passed = results.filter((r) => r.ok).length; const total = results.length; - console.log(`\n${"─".repeat(48)}`); + console.log(`\n${"-".repeat(48)}`); console.log(` ${passed}/${total} checks passed`); if (passed < total) { console.log(`\n Failed:`); diff --git a/typescript/src/config/settings.ts b/typescript/src/config/settings.ts index c48b383..30f368b 100644 --- a/typescript/src/config/settings.ts +++ b/typescript/src/config/settings.ts @@ -18,8 +18,6 @@ function parseBool(envKey: string, val: string): boolean { function parseFloat_(envKey: string, val: string): number { if (val.trim() === "") throw new Error(`Invalid value for ${envKey}: (empty string). Expected float.`); const n = Number(val); - // Reject NaN and Infinity: an infinite timeout/delay would disable the very - // limit it configures, and a negative duration is never meaningful here. if (!Number.isFinite(n) || n < 0) { throw new Error(`Invalid value for ${envKey}: ${val}. Expected a non-negative finite float.`); } @@ -30,8 +28,6 @@ function parseInt_(envKey: string, val: string): number { if (val.trim() === "") throw new Error(`Invalid value for ${envKey}: (empty string). Expected int.`); if (!/^-?\d+$/.test(val.trim())) throw new Error(`Invalid value for ${envKey}: ${val}. Expected int.`); const n = Number(val); - // Every integer setting (session/buffer/queue limits) must be non-negative; a - // negative value such as MAX_SESSIONS=-1 would block all session creation. if (n < 0) throw new Error(`Invalid value for ${envKey}: ${val}. Expected a non-negative integer.`); return n; } @@ -94,7 +90,6 @@ export class Settings { } static fromEnv(): Settings { - // Mutable partial — strips readonly so we can build the object incrementally. const overrides: { -readonly [K in keyof Settings]?: Settings[K] } = {}; const floatFields: Array<[keyof Settings, string]> = [ @@ -149,11 +144,6 @@ export class Settings { let _cachedSettings: Settings | null = null; -/** - * Build and cache settings from the environment. Wraps any parsing error with - * context so a misconfigured env var produces an actionable startup message - * instead of a raw error thrown from module initialisation. - */ export function initSettings(): Settings { try { _cachedSettings = Settings.fromEnv(); @@ -164,7 +154,6 @@ export function initSettings(): Settings { return _cachedSettings; } -/** Return the cached settings, initialising them lazily on first access. */ export function getSettings(): Settings { return _cachedSettings ?? initSettings(); } diff --git a/typescript/src/constants.ts b/typescript/src/constants.ts index ff27c14..8fb51eb 100644 --- a/typescript/src/constants.ts +++ b/typescript/src/constants.ts @@ -1,30 +1,22 @@ -// Command completion timing -export const MAX_COMMAND_COMPLETION_WINDOW = 0.1; // seconds +export const MAX_COMMAND_COMPLETION_WINDOW = 0.1; -// Process management export const PROCESS_SHUTDOWN_TIMEOUT = 2.0; export const FORCE_EXIT_DELAY_SECONDS = 2; -// Context display limits export const RECENT_OUTPUT_LINES = 20; export const LAST_COMMANDS_COUNT = 5; -// Memory conversion export const BYTES_TO_MB = 1024 * 1024; -// Buffer management export const UTILIZATION_PERCENTAGE_BASE = 100; export const LARGE_BUFFER_THRESHOLD = 10 * 1024 * 1024; export const SIGNIFICANT_LOG_THRESHOLD = 100_000; -// Performance optimization export const CHUNK_JOIN_THRESHOLD = 100; -// I/O logging thresholds export const LARGE_IO_THRESHOLD = 10_000; export const SLOW_OPERATION_THRESHOLD = 1.0; -// Cap on retained per-session command history entries. Bounds memory on -// long-lived sessions; the oldest entries are dropped once the cap is exceeded. +// Bounds memory on long-lived sessions; oldest entries are dropped when +// exceeded. export const MAX_COMMAND_HISTORY = 1000; - diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index a84c539..2ee93df 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -27,13 +27,9 @@ export interface CreateSessionOptions { /** * Manages OpenROAD subprocess lifecycle and interactive sessions. * - * Node.js is single-threaded, so no reentrant lock is needed for plain state - * access (Python used asyncio.Lock + the GIL). The async-mutex `cleanupLock` - * serialises the multi-await cleanup/creation sections so concurrent callers - * cannot interleave session-map mutations across await points. - * - * The module exports a shared `manager` singleton; the class is exported too so - * tests can construct isolated instances. + * The async-mutex `cleanupLock` serialises the multi-await cleanup/creation + * sections so concurrent callers cannot interleave session-map mutations + * across await points. */ export class OpenROADManager { private readonly logger = getLogger("manager"); @@ -73,8 +69,8 @@ export class OpenROADManager { this.sessions.set(sessionId, null); try { - // Match Python's `buffer_size or default`: 0 (and undefined) fall back - // to the default so a zero-capacity buffer can't silently drop all output. + // 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); @@ -92,8 +88,8 @@ export class OpenROADManager { async executeCommand(sessionId: string, command: string, timeoutMs?: number): Promise { const session = this._getSession(sessionId); - // Match Python's `timeout_ms or default`: 0 (and undefined) fall back to the - // configured default rather than becoming an instant timeout. + // 0 (and undefined) fall back to the default rather than becoming an + // instant timeout. const actualTimeout = timeoutMs && timeoutMs > 0 ? timeoutMs : this.defaultTimeoutMs; await session.sendCommand(command); @@ -121,8 +117,7 @@ export class OpenROADManager { async terminateSession(sessionId: string, force = false): Promise { const session = this._getSession(sessionId); - // terminate() already tears down the PTY and stops the writer task. We do - // not also call cleanup() here: cleanup() clears the output buffer, which + // Do not call cleanup() here: cleanup() clears the output buffer, which // would discard final output a concurrent reader may still need. The // session is dropped from the map below, so its buffer is GC'd anyway. await session.terminate(force); @@ -134,9 +129,9 @@ export class OpenROADManager { } async terminateAllSessions(force = false): Promise { - // Only initialized sessions are terminable. Null placeholders belong to an - // in-flight createSession (which resolves or removes them itself), so - // terminating them would throw "still being created" and be lost. + // Skip null placeholders: they belong to an in-flight createSession + // (which resolves or removes them itself), so terminating them would + // throw "still being created" and be lost. const sessionIds = this._initializedSessions().map(([sid]) => sid); if (sessionIds.length === 0) return 0; @@ -189,8 +184,8 @@ export class OpenROADManager { } } - // Capture counts after the async loop so the snapshot reflects the final - // state rather than a stale pre-loop value. + // Snapshot counts after the async loop so the result reflects the + // post-cleanup state. const totalSessions = this.sessions.size; const activeSessions = this.getActiveSessionCount(); const terminatedSessions = totalSessions - activeSessions; @@ -243,8 +238,6 @@ export class OpenROADManager { return this._countActive(); } - // internals - private _countActive(): number { let count = 0; for (const session of this.sessions.values()) { @@ -282,10 +275,9 @@ export class OpenROADManager { for (const [sessionId, session] of this._initializedSessions()) { if (!session.checkAlive()) { - // Measure from the actual death time. lastActivity is the last command, - // which for a long-idle session is far earlier and would trip the - // force-cleanup timer immediately. Fall back to lastActivity only if the - // death timestamp is somehow unset. + // Measure from death time, not lastActivity: a long-idle session + // dies far after its last command, which would trip force-cleanup + // immediately. const deathTime = (session.terminatedAt ?? session.lastActivity).getTime(); const timeSinceDeath = (now - deathTime) / 1000; terminated.push([sessionId, session, timeSinceDeath > FORCE_CLEANUP_AFTER_SECONDS]); @@ -329,5 +321,4 @@ export class OpenROADManager { } } -/** Shared process-wide manager instance used by the MCP tools. */ export const manager = new OpenROADManager(); diff --git a/typescript/src/interactive/buffer.ts b/typescript/src/interactive/buffer.ts index bc33e60..80435d6 100644 --- a/typescript/src/interactive/buffer.ts +++ b/typescript/src/interactive/buffer.ts @@ -37,8 +37,8 @@ export class CircularBuffer { this._totalSize -= old.length; } - // A single chunk that still exceeds maxSize is truncated to the last - // maxSize bytes so the buffer never permanently exceeds its capacity. + // A single chunk larger than maxSize is truncated to its last maxSize + // bytes so capacity is never permanently exceeded. if (this._totalSize > this.maxSize) { const chunk = this._chunks[0]!; this._chunks[0] = chunk.slice(chunk.length - this.maxSize); @@ -96,9 +96,9 @@ export class CircularBuffer { }; // Re-check _dataAvailable under the mutex: runExclusive is async, so - // append() can fire between the fast-path check above and the push below, - // set _dataAvailable = true, drain an empty _resolvers, and release — - // leaving wakeUp unnoticed and the caller waiting the full timeout. + // append() can fire between the fast-path check above and the push + // below, drain an empty _resolvers, and release, leaving wakeUp + // unnoticed and the caller waiting the full timeout. this._mutex.runExclusive(() => { if (this._dataAvailable) { wakeUp(true); diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index ade87b2..4b5d81d 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -15,7 +15,6 @@ export class PtyHandler { constructor(private readonly _settings: Settings = getSettings()) {} - /** PID of the underlying PTY process, or null if no process is active. */ get pid(): number | null { return this._ptyProcess?.pid ?? null; } @@ -89,9 +88,9 @@ export class PtyHandler { this._alive = true; this._exitCode = null; - // Register the exit handler before onData so a fast-exiting process can - // never slip its exit event through before we are listening. The guard - // keeps the handler idempotent against a double-delivered exit. + // Register exit before onData so a fast-exiting process cannot slip + // its exit event through before we are listening. The guard keeps the + // handler idempotent against a double-delivered exit. this._exitDisposable = this._ptyProcess.onExit(({ exitCode }) => { if (!this._alive && this._exitCode !== null) return; this._alive = false; @@ -123,8 +122,8 @@ export class PtyHandler { isProcessAlive(): boolean { if (!this._alive || !this._ptyProcess) return false; - // Defensive liveness probe: if the exit event was somehow missed, signal 0 - // detects a dead/reaped pid (ESRCH) so `_alive` cannot stay true forever. + // Defensive liveness probe in case the exit event was missed; signal 0 + // detects a dead/reaped pid via ESRCH. try { process.kill(this._ptyProcess.pid, 0); return true; @@ -135,8 +134,6 @@ export class PtyHandler { } async waitForExit(timeoutMs?: number): Promise { - // Check the recorded exit code first so callers that arrive after cleanup() - // still get the real code rather than null. if (this._exitCode !== null) return this._exitCode; if (!this._ptyProcess) return null; @@ -191,7 +188,7 @@ export class PtyHandler { try { await this.terminateProcess(true); } catch { - // Best effort - don't let terminate errors prevent state reset + // best effort } } @@ -205,7 +202,7 @@ export class PtyHandler { this._alive = false; this._dataDisposable = null; this._exitDisposable = null; - // Keep _exitCode so a late waitForExit() caller still sees the real exit - // code. createSession() resets it to null when the handler is reused. + // Preserve _exitCode so a late waitForExit() caller still sees the real + // exit code; createSession() resets it on reuse. } } diff --git a/typescript/src/main.ts b/typescript/src/main.ts index e228fb8..697dd4e 100644 --- a/typescript/src/main.ts +++ b/typescript/src/main.ts @@ -1,8 +1,7 @@ import { initSettings } from "./config/settings.js"; -// Eagerly initialise settings at startup so a misconfigured environment variable -// is reported with useful context and a non-zero exit code, rather than crashing -// later from inside module initialisation when settings are first accessed. +// Initialise settings up front so a misconfigured env var fails fast with +// context rather than crashing later from inside module initialisation. try { initSettings(); } catch (e) { diff --git a/typescript/src/tools/base.ts b/typescript/src/tools/base.ts index 7b34706..83ccab0 100644 --- a/typescript/src/tools/base.ts +++ b/typescript/src/tools/base.ts @@ -5,10 +5,9 @@ function camelToSnakeKey(key: string): string { } /** - * Recursively converts camelCase object keys to snake_case. - * Idempotent on already-snake_case strings (no uppercase → no change), - * so opaque snake_case payloads (SessionDetailedMetrics, ManagerMetrics, - * CommandHistoryEntry) pass through unchanged. + * Recursively convert camelCase object keys to snake_case. Idempotent on + * already-snake_case keys, so opaque snake_case payloads pass through + * unchanged. */ export function toSnakeCase(value: unknown): unknown { if (Array.isArray(value)) return value.map(toSnakeCase); @@ -24,11 +23,9 @@ export function toSnakeCase(value: unknown): unknown { } /** - * Base class for all MCP tool implementations. - * - * Provides the manager dependency and a serialization helper that converts the - * camelCase domain model to the snake_case MCP wire format in one place. - * Each subclass declares its own typed execute() signature. + * Base class for MCP tool implementations. Provides the manager dependency + * and a serialization helper that converts the camelCase domain model to the + * snake_case wire format. */ export abstract class BaseTool { protected constructor(protected readonly manager: OpenROADManager) {} diff --git a/typescript/src/tools/interactive.ts b/typescript/src/tools/interactive.ts index ba0cc99..c153fc6 100644 --- a/typescript/src/tools/interactive.ts +++ b/typescript/src/tools/interactive.ts @@ -25,17 +25,12 @@ import { BaseTool, toSnakeCase } from "./base.js"; const logger = getLogger("tools.interactive"); -// --------------------------------------------------------------------------- -// Module-level helpers -// --------------------------------------------------------------------------- - -/** Emulate Python's repr() for simple strings: single-quoted with escaping. */ +/** Single-quoted Python-style repr for embedding strings in error messages. */ function pyRepr(s: string): string { const escaped = s.replace(/\\/g, "\\\\").replace(/'/g, "\\'"); return `'${escaped}'`; } -/** Build a blank InteractiveExecResult skeleton for error paths. */ function blankExecResult( sessionId: string | null, error: string, @@ -51,10 +46,6 @@ function blankExecResult( }; } -/** - * Returns an InteractiveExecResult representing a session-not-found condition. - * Error message matches the Python server byte-for-byte. - */ function sessionNotFoundExecResult( sessionId: string | null, error: unknown, @@ -70,10 +61,6 @@ function sessionNotFoundExecResult( }; } -/** - * Build and serialize a blocked-command result. - * Matches Python's _blocked_error() output exactly, including repr() quoting. - */ function blockedError( command: string, blockedVerb: string, @@ -93,9 +80,8 @@ function blockedError( } /** - * Gate a command through the Tcl whitelist when WHITELIST_ENABLED is set. - * Returns a serialised blocked-error JSON string when the command is rejected, - * or null when it is allowed (or when the whitelist is disabled). + * Returns a serialised blocked-error JSON string when the command is rejected + * by the Tcl whitelist, or null when it is allowed or the whitelist is off. */ function applyWhitelist( command: string, @@ -114,10 +100,6 @@ function applyWhitelist( return null; } -// --------------------------------------------------------------------------- -// Tool classes -// --------------------------------------------------------------------------- - /** Read-only query tool: report_*, get_*, check_*, sta, help, version, etc. */ export class QueryShellTool extends BaseTool { constructor(manager: OpenROADManager) { @@ -146,8 +128,8 @@ export class QueryShellTool extends BaseTool { ); return this.formatResult(result as unknown as Record); } catch (e) { - // Auto-created sessions must be cleaned up when executeCommand throws to - // avoid leaking the session. + // Tear down an auto-created session so executeCommand failures do not + // leak it. if (sid === null && resolvedId !== null) { this.manager.terminateSession(resolvedId, true).catch(() => { /* best effort */ }); } @@ -234,7 +216,6 @@ export class ExecShellTool extends BaseTool { } } -/** Lists all active and terminated sessions tracked by the manager. */ export class ListSessionsTool extends BaseTool { constructor(manager: OpenROADManager) { super(manager); @@ -261,7 +242,6 @@ export class ListSessionsTool extends BaseTool { } } -/** Creates a new OpenROAD interactive session. */ export class CreateSessionTool extends BaseTool { constructor(manager: OpenROADManager) { super(manager); @@ -299,7 +279,6 @@ export class CreateSessionTool extends BaseTool { } } -/** Terminates an existing session by ID. */ export class TerminateSessionTool extends BaseTool { constructor(manager: OpenROADManager) { super(manager); @@ -347,7 +326,6 @@ export class TerminateSessionTool extends BaseTool { } } -/** Returns detailed metrics for a single session. */ export class InspectSessionTool extends BaseTool { constructor(manager: OpenROADManager) { super(manager); @@ -381,7 +359,6 @@ export class InspectSessionTool extends BaseTool { } } -/** Returns the command history for a session, with optional limit and search. */ export class SessionHistoryTool extends BaseTool { constructor(manager: OpenROADManager) { super(manager); @@ -422,7 +399,6 @@ export class SessionHistoryTool extends BaseTool { } } -/** Returns aggregate metrics across all sessions managed by the manager. */ export class SessionMetricsTool extends BaseTool { constructor(manager: OpenROADManager) { super(manager); @@ -447,5 +423,4 @@ export class SessionMetricsTool extends BaseTool { } } -// Backwards-compat alias matching Python's InteractiveShellTool = QueryShellTool export const InteractiveShellTool = QueryShellTool; diff --git a/typescript/src/tools/report_images.ts b/typescript/src/tools/report_images.ts index 2be681b..8e68663 100644 --- a/typescript/src/tools/report_images.ts +++ b/typescript/src/tools/report_images.ts @@ -1,5 +1,4 @@ import fs from "node:fs"; -import os from "node:os"; import path from "node:path"; import sharp from "sharp"; import type { OpenROADManager } from "../core/manager.js"; @@ -20,17 +19,9 @@ import { BaseTool } from "./base.js"; const logger = getLogger("tools.report_images"); -// --------------------------------------------------------------------------- -// Constants -// --------------------------------------------------------------------------- - const MAX_BASE64_SIZE_KB = 15; const MAX_IMAGE_SIZE_MB = 50; -// --------------------------------------------------------------------------- -// Image type mapping (exact copy of Python dict) -// --------------------------------------------------------------------------- - const IMAGE_TYPE_MAPPING: Record = { cts_clk: "clock_visualization", cts_clk_layout: "clock_layout", @@ -45,13 +36,9 @@ const IMAGE_TYPE_MAPPING: Record = { final_routing: "routing_visualization", }; -// --------------------------------------------------------------------------- -// Helper functions -// --------------------------------------------------------------------------- - /** - * Derive the image stage and semantic type from a filename. - * Returns ["unknown", "unknown"] for files with no underscore or unrecognised keys. + * Derive the image stage and semantic type from a filename. Returns + * ["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)); @@ -69,10 +56,6 @@ export function classifyImageType(filename: string): [string, string] { return [stage, type]; } -/** - * Verify that `platform` and `design` are known in the current ORFS configuration. - * Throws ValidationError when either is not found. - */ export function validatePlatformDesign(platform: string, design: string): void { const settings = getSettings(); const platforms = settings.platforms; @@ -89,10 +72,6 @@ export function validatePlatformDesign(platform: string, design: string): void { } } -/** - * Resolve and validate the reports base path and per-run sub-directory. - * Returns [reportsBase, runPath] as absolute path strings. - */ function resolveRunPath( platform: string, design: string, @@ -107,7 +86,6 @@ function resolveRunPath( return [reportsBase, runPath]; } -/** List available run slugs in reportsBase for error messages. */ function availableRuns(reportsBase: string): string[] { try { return fs @@ -119,16 +97,13 @@ function availableRuns(reportsBase: string): string[] { } } -/** - * Recursively find all .webp files under `dir`, returning absolute paths. - * Requires Node.js ≥ 20 for the `recursive` option on readdirSync. - */ +/** Requires Node.js 20+ for the `recursive` option on readdirSync. */ function findWebpFiles(dir: string): string[] { const entries = fs.readdirSync(dir, { recursive: true, withFileTypes: true }); return entries .filter((e) => e.isFile() && e.name.endsWith(".webp")) .map((e) => { - // `parentPath` is available in Node 20.12+; `path` is the older alias. + // `parentPath` is Node 20.12+; `path` is the older alias. const parent = (e as unknown as { parentPath?: string; path?: string }) .parentPath ?? (e as unknown as { path: string }).path; return path.join(parent, e.name); @@ -147,9 +122,9 @@ interface CompressResult { } /** - * Load an image and compress it to fit within `maxSizeKb` of base64 output. - * Uses sharp for resizing (lanczos3) and WebP encoding (quality=85). - * Falls back to returning raw bytes when sharp fails, with null dimensions. + * Compress an image to fit within `maxSizeKb` of base64 output using sharp + * (lanczos3 resize, WebP quality 85). Falls back to raw bytes with null + * dimensions when sharp fails. */ async function loadAndCompressImage( imagePath: string, @@ -227,10 +202,6 @@ async function loadAndCompressImage( } } -// --------------------------------------------------------------------------- -// Tool classes -// --------------------------------------------------------------------------- - /** Lists .webp report images for a specific platform/design/run. */ export class ListReportImagesTool extends BaseTool { constructor(manager: OpenROADManager) { @@ -316,7 +287,6 @@ export class ListReportImagesTool extends BaseTool { total++; } - // Sort each stage bucket by filename for (const key of Object.keys(imagesByStage)) { imagesByStage[key] = (imagesByStage[key] as Array<{ filename: string }>).sort((a, b) => a.filename.localeCompare(b.filename), diff --git a/typescript/src/utils/ansi_decoder.ts b/typescript/src/utils/ansi_decoder.ts index 91a2271..6610a1f 100644 --- a/typescript/src/utils/ansi_decoder.ts +++ b/typescript/src/utils/ansi_decoder.ts @@ -1,19 +1,17 @@ import stripAnsi from "strip-ansi"; +// Specific private-mode codes are listed before the generic private-mode +// catch-all so they match first. The `?` in the generic patterns is escaped, +// preventing single-char escapes like \x1bh from matching there. const ESCAPE_SEQUENCES: Record = { - // Private mode sequences (DECSET/DECRST). Specific codes are listed first so - // they match before the generic private-mode catch-all below. "\\x1b\\[\\?2004h": "Enable bracketed paste mode", "\\x1b\\[\\?2004l": "Disable bracketed paste mode", "\\x1b\\[\\?1049h": "Enable alternative screen buffer", "\\x1b\\[\\?1049l": "Disable alternative screen buffer", "\\x1b\\[\\?25h": "Show cursor", "\\x1b\\[\\?25l": "Hide cursor", - // Generic private-mode set/reset. The `?` is escaped so `[?` is mandatory, - // preventing single-char escapes like \x1bh / \x1bl from matching here. "\\x1b\\[\\?\\d*h": "Enable terminal mode", "\\x1b\\[\\?\\d*l": "Disable terminal mode", - // Cursor movement "\\x1b\\[\\d*A": "Move cursor up", "\\x1b\\[\\d*B": "Move cursor down", "\\x1b\\[\\d*C": "Move cursor right", @@ -26,7 +24,6 @@ const ESCAPE_SEQUENCES: Record = { "\\x1b\\[0K": "Clear line from cursor to end", "\\x1b\\[1K": "Clear line from start to cursor", "\\x1b\\[2K": "Clear entire line", - // Text formatting "\\x1b\\[0m": "Reset all formatting", "\\x1b\\[1m": "Bold text", "\\x1b\\[2m": "Dim text", @@ -36,7 +33,6 @@ const ESCAPE_SEQUENCES: Record = { "\\x1b\\[7m": "Reverse video", "\\x1b\\[8m": "Hidden text", "\\x1b\\[9m": "Strikethrough text", - // Colors (foreground) "\\x1b\\[30m": "Black text", "\\x1b\\[31m": "Red text", "\\x1b\\[32m": "Green text", @@ -45,7 +41,6 @@ const ESCAPE_SEQUENCES: Record = { "\\x1b\\[35m": "Magenta text", "\\x1b\\[36m": "Cyan text", "\\x1b\\[37m": "White text", - // Colors (background) "\\x1b\\[40m": "Black background", "\\x1b\\[41m": "Red background", "\\x1b\\[42m": "Green background", @@ -56,21 +51,17 @@ const ESCAPE_SEQUENCES: Record = { "\\x1b\\[47m": "White background", }; -// Matches the common ANSI escape sequence families so that non-CSI sequences -// (OSC, charset designation, single/two-char escapes) are detected in every -// mode rather than leaking through as raw bytes. Order matters: longer/anchored -// alternatives come first so they win at a given match position. -// 1. OSC: ESC ] ... (BEL | ST) -// 2. CSI: ESC [ params final -// 3. Charset/desig.: ESC ( | ) | # +// Order matters: longer/anchored alternatives come first so they win at a +// given match position. +// 1. OSC: ESC ] ... (BEL | ST) +// 2. CSI: ESC [ params final +// 3. Charset/desig.: ESC ( | ) | # // 4. Single/two-char: ESC const ESCAPE_PATTERN = /\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)|\x1b\[[0-9;?]*[a-zA-Z]|\x1b[()#][0-9A-Za-z]|\x1b[=>NMcDEH78]/g; const VALID_MODES = ["remove", "annotate", "preserve", "decode"]; -// Pre-compile the escape-sequence patterns once at module load instead of -// allocating ~45 RegExp objects on every decodeEscapeSequence() call. const COMPILED_SEQUENCES: Array<[RegExp, string]> = Object.entries(ESCAPE_SEQUENCES).map( ([pattern, description]) => [new RegExp(`^${pattern}`), description], ); @@ -115,9 +106,8 @@ export class ANSIDecoder { if (mode === "annotate") { let result = text; for (const seq of new Set(sequences)) { - // Function replacement: the value is inserted literally, so `$&`/`$1` - // inside an OSC sequence body cannot be reinterpreted as a replacement - // pattern and corrupt the output. + // Use a function replacement so `$&`/`$1` inside an OSC sequence + // body cannot be reinterpreted as a replacement pattern. const annotation = `[${ANSIDecoder.decodeEscapeSequence(seq)}]`; result = result.replaceAll(seq, () => annotation); } diff --git a/typescript/src/utils/logging.ts b/typescript/src/utils/logging.ts index 3266ea3..d8f3f15 100644 --- a/typescript/src/utils/logging.ts +++ b/typescript/src/utils/logging.ts @@ -1,32 +1,21 @@ import pino from "pino"; import { getSettings } from "../config/settings.js"; -/** - * Pino-based logging for the MCP server. - * - * IMPORTANT: all log output goes to stderr (file descriptor 2). stdout is - * reserved exclusively for the MCP stdio transport - writing logs there would - * corrupt the JSON-RPC protocol stream. - * - * The root level is initialised from settings (env-driven) at module load so - * child loggers created eagerly (e.g. the module-level OpenROADManager - * singleton) honour the configured level without depending on setupLogging() - * being called first. setupLogging() mutates the root level for loggers created - * afterwards; note that pino child loggers capture their level at creation time - * and do not dynamically follow the parent. - */ +// All log output goes to stderr (fd 2). stdout is reserved for the MCP stdio +// transport and any log writes there would corrupt the JSON-RPC stream. +// pino child loggers capture their level at creation, so the root level is +// initialised from settings at module load to honour the configured level for +// eagerly created singletons. function createRoot(level: string): pino.Logger { return pino({ name: "openroad_mcp", level: level.toLowerCase() }, pino.destination(2)); } let rootLogger: pino.Logger = createRoot(getSettings().LOG_LEVEL); -/** Configure the root log level. Call once at startup before heavy logging. */ export function setupLogging(level: string): void { rootLogger.level = level.toLowerCase(); } -/** Return a child logger namespaced under `openroad_mcp.`. */ export function getLogger(name: string): pino.Logger { return rootLogger.child({ module: name }); } diff --git a/typescript/src/utils/path_security.ts b/typescript/src/utils/path_security.ts index 2b60008..10507e1 100644 --- a/typescript/src/utils/path_security.ts +++ b/typescript/src/utils/path_security.ts @@ -26,10 +26,10 @@ export function validateSafePathContainment(targetPath: string, basePath: string if ((e as NodeJS.ErrnoException).code !== "ENOENT") { throw new ValidationError(`Failed to resolve ${context} path: ${e}`); } - // Walk up to find the longest existing prefix, resolve its symlinks, then - // re-append the non-existent suffix. A plain path.resolve() is unsafe here - // because it won't resolve symlinks in existing parent directories, allowing - // e.g. base/evil_link/nonexistent to escape containment at runtime. + // Walk up to the longest existing prefix, resolve its symlinks, then + // re-append the non-existent suffix. A plain path.resolve() would not + // resolve symlinks in existing parent directories, allowing e.g. + // base/evil_link/nonexistent to escape containment at runtime. const suffix: string[] = []; let current = path.resolve(targetPath); for (;;) { From 461407aea57115606d87e007fe0fc488f149ab93 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Wed, 17 Jun 2026 20:52:24 -0600 Subject: [PATCH 35/44] add interactive tool wire-format snapshots so CI snapshot tests pass --- .../__tests__/tools/__snapshots__/interactive.test.ts.snap | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 typescript/__tests__/tools/__snapshots__/interactive.test.ts.snap diff --git a/typescript/__tests__/tools/__snapshots__/interactive.test.ts.snap b/typescript/__tests__/tools/__snapshots__/interactive.test.ts.snap new file mode 100644 index 0000000..c6a9798 --- /dev/null +++ b/typescript/__tests__/tools/__snapshots__/interactive.test.ts.snap @@ -0,0 +1,5 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`Snapshots: wire format stability > ListSessionsTool with sessions 1`] = `"{"sessions":[{"session_id":"session-1","created_at":"2024-01-01T00:00:00.000Z","is_alive":true,"command_count":5,"buffer_size":1024,"uptime_seconds":100,"state":"active","error":null}],"total_count":1,"active_count":1,"error":null}"`; + +exports[`Snapshots: wire format stability > QueryShellTool success output 1`] = `"{"output":"test output","session_id":"session-1","timestamp":"2024-01-01T00:00:00.000Z","execution_time":0.1,"command_count":1,"buffer_size":0,"error":null}"`; From 973b6fa9089fa67e31a6b38cf1fc2bb7c80e5aae Mon Sep 17 00:00:00 2001 From: kartikloops Date: Fri, 26 Jun 2026 21:22:02 -0600 Subject: [PATCH 36/44] clamp brace depth and gate quotes on word boundaries so an unbalanced } or mid-word " cannot hide a trailing statement from the verb check --- .../config/command_whitelist.test.ts | 12 +++++++++++ typescript/src/config/command_whitelist.ts | 20 +++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/typescript/__tests__/config/command_whitelist.test.ts b/typescript/__tests__/config/command_whitelist.test.ts index 0384a66..fa64ade 100644 --- a/typescript/__tests__/config/command_whitelist.test.ts +++ b/typescript/__tests__/config/command_whitelist.test.ts @@ -241,6 +241,14 @@ describe("isQueryCommand", () => { it("semicolon inside braces is not a statement separator (finding 3)", () => { expect(isQueryCommand("report_checks {a; b}")).toEqual([true, null]); }); + + it("unbalanced close brace cannot hide a trailing exec (depth clamp)", () => { + expect(isQueryCommand("report_wns }; exec ls")).toEqual([false, "exec"]); + }); + + it("mid-word quote is literal and cannot hide a trailing exec", () => { + expect(isQueryCommand('set x a"b ; exec ls')).toEqual([false, "exec"]); + }); }); // isExecCommand @@ -304,6 +312,10 @@ describe("isExecCommand", () => { it("blocks a multiline command with one blocked verb", () => { expect(isExecCommand("global_placement\nsocket tcp localhost")).toEqual([false, "socket"]); }); + + it("unbalanced close brace cannot hide a trailing quit (depth clamp)", () => { + expect(isExecCommand("set x } ; quit")).toEqual([false, "quit"]); + }); }); // isCommandAllowed (backward-compat alias) diff --git a/typescript/src/config/command_whitelist.ts b/typescript/src/config/command_whitelist.ts index a940d1f..73a9077 100644 --- a/typescript/src/config/command_whitelist.ts +++ b/typescript/src/config/command_whitelist.ts @@ -190,26 +190,42 @@ function splitTclStatements(command: string): string[] { let depth = 0; let inQuote = false; let current = ""; + // In Tcl a `"` only opens a quoted string at the start of a word; a quote in + // the middle of a word (e.g. `a"b`) is a literal. Track word boundaries so an + // unbalanced mid-word quote cannot flip us into quote mode and swallow the + // separator that follows it. + let atWordStart = true; for (let i = 0; i < command.length; i++) { const ch = command[i]!; if (ch === "\\" && i + 1 < command.length) { current += ch + command[i + 1]!; i++; - } else if (ch === '"' && depth === 0) { + atWordStart = false; + } else if (ch === '"' && depth === 0 && (inQuote || atWordStart)) { inQuote = !inQuote; current += ch; + atWordStart = false; } else if (!inQuote && ch === "{") { depth++; current += ch; + atWordStart = false; } else if (!inQuote && ch === "}") { - depth--; + // Clamp at zero: an unbalanced close brace must not drive depth negative, + // which would make the `depth === 0` separator test below permanently + // false and hide every subsequent statement from the verb check. + if (depth > 0) depth--; current += ch; + atWordStart = false; } else if (!inQuote && depth === 0 && (ch === ";" || STATEMENT_SEPARATORS.has(ch))) { stmts.push(current); current = ""; + atWordStart = true; } else { current += ch; + // Whitespace and `[` (which begins a command substitution) start a new + // word, so a `"` immediately after them is again quote-significant. + atWordStart = ch === " " || ch === "\t" || ch === "["; } } if (current) stmts.push(current); From 4df16633e9f59211842eb2177e935c78e42f8c7d Mon Sep 17 00:00:00 2001 From: kartikloops Date: Fri, 26 Jun 2026 21:23:47 -0600 Subject: [PATCH 37/44] walk the report tree manually and skip symlinks so a linked dir or file cannot escape the contained run directory --- .../__tests__/tools/report_images.test.ts | 28 +++++++++++++++++++ typescript/src/tools/report_images.ts | 21 +++++++------- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/typescript/__tests__/tools/report_images.test.ts b/typescript/__tests__/tools/report_images.test.ts index 44a41c6..81c3d32 100644 --- a/typescript/__tests__/tools/report_images.test.ts +++ b/typescript/__tests__/tools/report_images.test.ts @@ -180,6 +180,34 @@ describe("ListReportImagesTool", () => { expect(result.images_by_stage).toHaveProperty("final"); }); + it("does not descend symlinked directories or list symlinked files", async () => { + const { flowPath, runPath } = createFixture("nangate45", "gcd", "run-123", [ + "cts_clk.webp", + ]); + // A directory of images outside the run, reachable only via symlinks. + const outside = path.join(tmpDir, "outside"); + fs.mkdirSync(outside, { recursive: true }); + fs.writeFileSync(path.join(outside, "final_all.webp"), Buffer.from("RIFF\x00\x00\x00\x00WEBP")); + try { + fs.symlinkSync(outside, path.join(runPath, "linkdir")); + fs.symlinkSync(path.join(outside, "final_all.webp"), path.join(runPath, "linkfile.webp")); + } catch { + // symlink creation may fail in some environments; skip gracefully. + return; + } + (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")); + // Only the real cts_clk.webp is found; nothing reached through a symlink. + expect(result.total_images).toBe(1); + expect(result.images_by_stage).toHaveProperty("cts"); + expect(result.images_by_stage).not.toHaveProperty("final"); + }); + it("filters images by stage", async () => { const { flowPath } = createFixture("nangate45", "gcd", "run-123", [ "cts_clk.webp", diff --git a/typescript/src/tools/report_images.ts b/typescript/src/tools/report_images.ts index 8e68663..f88ed2b 100644 --- a/typescript/src/tools/report_images.ts +++ b/typescript/src/tools/report_images.ts @@ -97,17 +97,18 @@ function availableRuns(reportsBase: string): string[] { } } -/** Requires Node.js 20+ for the `recursive` option on readdirSync. */ function findWebpFiles(dir: string): string[] { - const entries = fs.readdirSync(dir, { recursive: true, withFileTypes: true }); - return entries - .filter((e) => e.isFile() && e.name.endsWith(".webp")) - .map((e) => { - // `parentPath` is Node 20.12+; `path` is the older alias. - const parent = (e as unknown as { parentPath?: string; path?: string }) - .parentPath ?? (e as unknown as { path: string }).path; - return path.join(parent, e.name); - }); + 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(full); + } + } + return results; } interface CompressResult { From 0ba6e24e4d410b08ac61b4dba9c06ba210ea365a Mon Sep 17 00:00:00 2001 From: kartikloops Date: Fri, 26 Jun 2026 21:28:31 -0600 Subject: [PATCH 38/44] reject zero for env settings where it is degenerate (max sessions, queue/buffer/chunk sizes, command/idle timeouts) so a 0 cannot silently disable sessions or output --- typescript/__tests__/config/settings.test.ts | 26 +++++++++++++ typescript/src/config/settings.ts | 41 +++++++++++--------- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/typescript/__tests__/config/settings.test.ts b/typescript/__tests__/config/settings.test.ts index 799cf97..c85069f 100644 --- a/typescript/__tests__/config/settings.test.ts +++ b/typescript/__tests__/config/settings.test.ts @@ -135,6 +135,32 @@ describe("Settings", () => { expect(() => Settings.fromEnv()).toThrow("OPENROAD_MAX_SESSIONS"); }); + it("rejects zero for int fields that must be positive", () => { + for (const key of [ + "OPENROAD_MAX_SESSIONS", + "OPENROAD_SESSION_QUEUE_SIZE", + "OPENROAD_DEFAULT_BUFFER_SIZE", + "OPENROAD_READ_CHUNK_SIZE", + ]) { + process.env[key] = "0"; + expect(() => Settings.fromEnv()).toThrow(key); + delete process.env[key]; + } + }); + + it("rejects zero for float fields that must be positive", () => { + for (const key of ["OPENROAD_COMMAND_TIMEOUT", "OPENROAD_SESSION_IDLE_TIMEOUT"]) { + process.env[key] = "0"; + expect(() => Settings.fromEnv()).toThrow(key); + delete process.env[key]; + } + }); + + it("allows zero for COMMAND_COMPLETION_DELAY (no delay)", () => { + process.env["OPENROAD_COMMAND_COMPLETION_DELAY"] = "0"; + expect(Settings.fromEnv().COMMAND_COMPLETION_DELAY).toBe(0); + }); + it("rejects decimal strings for int fields", () => { process.env["OPENROAD_MAX_SESSIONS"] = "50.0"; expect(() => Settings.fromEnv()).toThrow("OPENROAD_MAX_SESSIONS"); diff --git a/typescript/src/config/settings.ts b/typescript/src/config/settings.ts index 30f368b..6368fc7 100644 --- a/typescript/src/config/settings.ts +++ b/typescript/src/config/settings.ts @@ -15,20 +15,24 @@ function parseBool(envKey: string, val: string): boolean { ); } -function parseFloat_(envKey: string, val: string): number { +function parseFloat_(envKey: string, val: string, allowZero: boolean): number { if (val.trim() === "") throw new Error(`Invalid value for ${envKey}: (empty string). Expected float.`); const n = Number(val); - if (!Number.isFinite(n) || n < 0) { - throw new Error(`Invalid value for ${envKey}: ${val}. Expected a non-negative finite float.`); + const bound = allowZero ? "non-negative" : "positive"; + if (!Number.isFinite(n) || n < 0 || (!allowZero && n === 0)) { + throw new Error(`Invalid value for ${envKey}: ${val}. Expected a ${bound} finite float.`); } return n; } -function parseInt_(envKey: string, val: string): number { +function parseInt_(envKey: string, val: string, allowZero: boolean): number { if (val.trim() === "") throw new Error(`Invalid value for ${envKey}: (empty string). Expected int.`); if (!/^-?\d+$/.test(val.trim())) throw new Error(`Invalid value for ${envKey}: ${val}. Expected int.`); const n = Number(val); - if (n < 0) throw new Error(`Invalid value for ${envKey}: ${val}. Expected a non-negative integer.`); + const bound = allowZero ? "non-negative" : "positive"; + if (n < 0 || (!allowZero && n === 0)) { + throw new Error(`Invalid value for ${envKey}: ${val}. Expected a ${bound} integer.`); + } return n; } @@ -92,16 +96,17 @@ export class Settings { static fromEnv(): Settings { const overrides: { -readonly [K in keyof Settings]?: Settings[K] } = {}; - const floatFields: Array<[keyof Settings, string]> = [ - ["COMMAND_TIMEOUT", "OPENROAD_COMMAND_TIMEOUT"], - ["COMMAND_COMPLETION_DELAY", "OPENROAD_COMMAND_COMPLETION_DELAY"], - ["SESSION_IDLE_TIMEOUT", "OPENROAD_SESSION_IDLE_TIMEOUT"], + // [field, envKey, allowZero] + const floatFields: Array<[keyof Settings, string, boolean]> = [ + ["COMMAND_TIMEOUT", "OPENROAD_COMMAND_TIMEOUT", false], + ["COMMAND_COMPLETION_DELAY", "OPENROAD_COMMAND_COMPLETION_DELAY", true], + ["SESSION_IDLE_TIMEOUT", "OPENROAD_SESSION_IDLE_TIMEOUT", false], ]; - const intFields: Array<[keyof Settings, string]> = [ - ["DEFAULT_BUFFER_SIZE", "OPENROAD_DEFAULT_BUFFER_SIZE"], - ["MAX_SESSIONS", "OPENROAD_MAX_SESSIONS"], - ["SESSION_QUEUE_SIZE", "OPENROAD_SESSION_QUEUE_SIZE"], - ["READ_CHUNK_SIZE", "OPENROAD_READ_CHUNK_SIZE"], + const intFields: Array<[keyof Settings, string, boolean]> = [ + ["DEFAULT_BUFFER_SIZE", "OPENROAD_DEFAULT_BUFFER_SIZE", false], + ["MAX_SESSIONS", "OPENROAD_MAX_SESSIONS", false], + ["SESSION_QUEUE_SIZE", "OPENROAD_SESSION_QUEUE_SIZE", false], + ["READ_CHUNK_SIZE", "OPENROAD_READ_CHUNK_SIZE", false], ]; const strFields: Array<[keyof Settings, string]> = [ ["LOG_LEVEL", "LOG_LEVEL"], @@ -109,13 +114,13 @@ export class Settings { ["ORFS_FLOW_PATH", "ORFS_FLOW_PATH"], ]; - for (const [field, envKey] of floatFields) { + for (const [field, envKey, allowZero] of floatFields) { const val = process.env[envKey]; - if (val !== undefined) (overrides as Record)[field] = parseFloat_(envKey, val); + if (val !== undefined) (overrides as Record)[field] = parseFloat_(envKey, val, allowZero); } - for (const [field, envKey] of intFields) { + for (const [field, envKey, allowZero] of intFields) { const val = process.env[envKey]; - if (val !== undefined) (overrides as Record)[field] = parseInt_(envKey, val); + if (val !== undefined) (overrides as Record)[field] = parseInt_(envKey, val, allowZero); } for (const [field, envKey] of strFields) { const val = process.env[envKey]; From bd682440b71b2b50808092e00b92c41e0d8f895f Mon Sep 17 00:00:00 2001 From: kartikloops Date: Fri, 26 Jun 2026 21:46:56 -0600 Subject: [PATCH 39/44] scan bracket substitutions per statement and skip comment/blank lines so a bracket inside a # comment is not rejected as an exec --- .../config/command_whitelist.test.ts | 12 ++++++++ typescript/src/config/command_whitelist.ts | 29 +++++++++++-------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/typescript/__tests__/config/command_whitelist.test.ts b/typescript/__tests__/config/command_whitelist.test.ts index fa64ade..9063a70 100644 --- a/typescript/__tests__/config/command_whitelist.test.ts +++ b/typescript/__tests__/config/command_whitelist.test.ts @@ -242,6 +242,18 @@ describe("isQueryCommand", () => { expect(isQueryCommand("report_checks {a; b}")).toEqual([true, null]); }); + it("allows a bracket inside a comment line (never executed)", () => { + expect(isQueryCommand("# harmless [exec ls]")).toEqual([true, null]); + }); + + it("allows a comment with a bracket after a readonly statement", () => { + expect(isQueryCommand("report_wns\n# harmless [exec ls]")).toEqual([true, null]); + }); + + it("still blocks a bracket when # is a mid-statement arg, not a comment", () => { + expect(isQueryCommand("report_checks # [exec ls]")).toEqual([false, "exec"]); + }); + it("unbalanced close brace cannot hide a trailing exec (depth clamp)", () => { expect(isQueryCommand("report_wns }; exec ls")).toEqual([false, "exec"]); }); diff --git a/typescript/src/config/command_whitelist.ts b/typescript/src/config/command_whitelist.ts index 73a9077..01b3af9 100644 --- a/typescript/src/config/command_whitelist.ts +++ b/typescript/src/config/command_whitelist.ts @@ -235,24 +235,29 @@ function splitTclStatements(command: string): string[] { /** * Iterate the verbs of a Tcl command string. * - * Two passes: - * 1. Statement verbs — the leading token of each statement. The split is - * Tcl-aware (skips separators inside quotes/braces) and treats `;` plus all - * Unicode line boundaries as separators so a \r cannot hide a command. - * 2. Bracket verbs — the word immediately following each `[` (bracket - * substitution). This catches `set x [exec ls]` where the outer verb `set` - * is safe but the substituted command `exec` is not. + * Per statement, in a Tcl-aware split (separators inside quotes/braces are + * ignored; `;` plus all Unicode line boundaries are separators so a \r cannot + * hide a command), two kinds of verb are yielded: + * 1. The statement verb — its leading token. + * 2. Bracket verbs — the word following each `[` (bracket substitution). This + * catches `set x [exec ls]` where the outer verb `set` is safe but the + * substituted command `exec` is not. + * + * Comment and blank statements (extractVerb returns null) are skipped entirely, + * including their brackets: a `#` comment is discarded at Tcl parse time and is + * never executed, so `# harmless [exec ls]` must not be rejected on the `exec`. + * Brackets inside brace-quoted arguments are still scanned, because commands + * like `expr`/`eval` re-evaluate brace contents, so they cannot be assumed inert. */ function* iterVerbs(command: string): Generator { for (const stmt of splitTclStatements(command)) { const verb = extractVerb(stmt); - if (verb !== null) { - yield verb; + if (verb === null) continue; + yield verb; + for (const match of stmt.matchAll(/\[\s*(?::+)?(\w+)/g)) { + yield match[1]!; } } - for (const match of command.matchAll(/\[\s*(?::+)?(\w+)/g)) { - yield match[1]!; - } } /** From eb4cdfd475636b7eae72534d4b79971d646c3be6 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Fri, 26 Jun 2026 21:49:52 -0600 Subject: [PATCH 40/44] treat EPERM from the liveness probe as alive so a pid we cannot signal is not misreported dead, only ESRCH counts as gone --- .../__tests__/interactive/pty_handler.test.ts | 18 ++++++++++++++++++ typescript/src/interactive/pty_handler.ts | 8 ++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/typescript/__tests__/interactive/pty_handler.test.ts b/typescript/__tests__/interactive/pty_handler.test.ts index 1dc4688..93f4aae 100644 --- a/typescript/__tests__/interactive/pty_handler.test.ts +++ b/typescript/__tests__/interactive/pty_handler.test.ts @@ -163,6 +163,24 @@ describe("PtyHandler", () => { mockPty._exit(0); expect(handler.isProcessAlive()).toBe(false); }); + + it("returns false when the probe throws ESRCH (pid gone)", async () => { + await handler.createSession(["echo"]); + const killSpy = vi.spyOn(process, "kill").mockImplementation(() => { + throw Object.assign(new Error("kill ESRCH"), { code: "ESRCH" }); + }); + expect(handler.isProcessAlive()).toBe(false); + killSpy.mockRestore(); + }); + + it("returns true when the probe throws EPERM (pid exists, not signalable)", async () => { + await handler.createSession(["echo"]); + const killSpy = vi.spyOn(process, "kill").mockImplementation(() => { + throw Object.assign(new Error("kill EPERM"), { code: "EPERM" }); + }); + expect(handler.isProcessAlive()).toBe(true); + killSpy.mockRestore(); + }); }); describe("waitForExit", () => { diff --git a/typescript/src/interactive/pty_handler.ts b/typescript/src/interactive/pty_handler.ts index 4b5d81d..c3adb28 100644 --- a/typescript/src/interactive/pty_handler.ts +++ b/typescript/src/interactive/pty_handler.ts @@ -123,11 +123,15 @@ export class PtyHandler { isProcessAlive(): boolean { if (!this._alive || !this._ptyProcess) return false; // Defensive liveness probe in case the exit event was missed; signal 0 - // detects a dead/reaped pid via ESRCH. + // sends nothing, it only tests the pid. try { process.kill(this._ptyProcess.pid, 0); return true; - } catch { + } catch (e) { + // EPERM means the pid exists but we may not signal it (e.g. re-parented + // or owned by another user) — the process is still alive. Only ESRCH (no + // such pid) and other failures mean it is gone. + if ((e as NodeJS.ErrnoException).code === "EPERM") return true; this._alive = false; return false; } From 6abbf0a4d6d34a60bc451c06620ee00e31667341 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Sun, 28 Jun 2026 21:32:56 -0600 Subject: [PATCH 41/44] author the metrics and command-history payloads in camelCase like every other domain model so snake_case is produced only by toSnakeCase at the wire boundary, not hand-written --- typescript/__tests__/core/manager.test.ts | 28 ++++---- .../__tests__/interactive/session.test.ts | 32 ++++----- .../__tests__/tools/interactive.test.ts | 26 +++---- typescript/src/core/manager.ts | 24 +++---- typescript/src/core/models.ts | 68 ++++++++++--------- typescript/src/interactive/session.ts | 46 ++++++------- 6 files changed, 113 insertions(+), 111 deletions(-) diff --git a/typescript/__tests__/core/manager.test.ts b/typescript/__tests__/core/manager.test.ts index 976356f..dd40abb 100644 --- a/typescript/__tests__/core/manager.test.ts +++ b/typescript/__tests__/core/manager.test.ts @@ -31,17 +31,17 @@ interface MockSession { function makeMockSession(sessionId: string, alive = true): MockSession { const metrics: SessionDetailedMetrics = { - session_id: sessionId, + sessionId, state: SessionState.ACTIVE, - is_alive: alive, - created_at: new Date().toISOString(), - last_activity: new Date().toISOString(), - uptime_seconds: 1, - idle_seconds: 0, - commands: { total_executed: 3, current_count: 3, history_length: 3 }, - performance: { total_cpu_time: 0.5, peak_memory_mb: 10, current_memory_mb: 8 }, - buffer: { current_size: 0, max_size: 1024, utilization_percent: 0 }, - timeout: { configured_seconds: null, is_timed_out: false }, + isAlive: alive, + createdAt: new Date().toISOString(), + lastActivity: new Date().toISOString(), + uptimeSeconds: 1, + idleSeconds: 0, + commands: { totalExecuted: 3, currentCount: 3, historyLength: 3 }, + performance: { totalCpuTime: 0.5, peakMemoryMb: 10, currentMemoryMb: 8 }, + buffer: { currentSize: 0, maxSize: 1024, utilizationPercent: 0 }, + timeout: { configuredSeconds: null, isTimedOut: false }, }; return { @@ -224,7 +224,7 @@ describe("OpenROADManager", () => { await manager.createSession({ sessionId: "s1" }); const metrics = await manager.inspectSession("s1"); expect(created[0]!.getDetailedMetrics).toHaveBeenCalledOnce(); - expect(metrics.session_id).toBe("s1"); + expect(metrics.sessionId).toBe("s1"); }); it("getSessionHistory forwards limit and search", async () => { @@ -239,9 +239,9 @@ describe("OpenROADManager", () => { await manager.createSession({ sessionId: "s1" }); await manager.createSession({ sessionId: "s2" }); const metrics = await manager.sessionMetrics(); - expect(metrics.manager.total_sessions).toBe(2); - expect(metrics.manager.active_sessions).toBe(2); - expect(metrics.aggregate.total_commands).toBe(6); // 3 per mock session + expect(metrics.manager.totalSessions).toBe(2); + expect(metrics.manager.activeSessions).toBe(2); + expect(metrics.aggregate.totalCommands).toBe(6); // 3 per mock session expect(metrics.sessions).toHaveLength(2); }); }); diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index ce6a575..5a5a8c3 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -609,8 +609,8 @@ describe("InteractiveSession", () => { expect(session.commandHistory).toHaveLength(1); expect(session.commandHistory[0]!.command).toBe("report_wns"); - expect(session.commandHistory[0]!.command_number).toBe(1); - expect(typeof session.commandHistory[0]!.execution_start).toBe("number"); + expect(session.commandHistory[0]!.commandNumber).toBe(1); + expect(typeof session.commandHistory[0]!.executionStart).toBe("number"); expect(session.totalCommandsExecuted).toBe(1); expect(session.lastActivity.getTime()).toBeGreaterThanOrEqual(before); }); @@ -625,8 +625,8 @@ describe("InteractiveSession", () => { await session.sendCommand("cmd_b"); await session.readOutput(50); - expect(session.commandHistory[0]!.execution_time).toBeDefined(); - expect(session.commandHistory[1]!.execution_time).toBeDefined(); + expect(session.commandHistory[0]!.executionTime).toBeDefined(); + expect(session.commandHistory[1]!.executionTime).toBeDefined(); }); it("bounds commandHistory at MAX_COMMAND_HISTORY, dropping the oldest", async () => { @@ -640,7 +640,7 @@ describe("InteractiveSession", () => { expect(s.commandHistory).toHaveLength(MAX_COMMAND_HISTORY); // Oldest entries dropped: first retained command_number is total - MAX + 1. - expect(s.commandHistory[0]!.command_number).toBe(total - MAX_COMMAND_HISTORY + 1); + expect(s.commandHistory[0]!.commandNumber).toBe(total - MAX_COMMAND_HISTORY + 1); await s.cleanup(); }); @@ -676,13 +676,13 @@ describe("InteractiveSession", () => { await session.sendCommand("report_wns"); const m = await session.getDetailedMetrics(); - expect(m.session_id).toBe("test-session-1"); - expect(m.is_alive).toBe(true); - expect(m.commands.total_executed).toBe(1); - expect(m.commands.history_length).toBe(1); - expect(m.buffer.max_size).toBe(1024); - expect(m.timeout.configured_seconds).toBeNull(); - expect(m.timeout.is_timed_out).toBe(false); + expect(m.sessionId).toBe("test-session-1"); + expect(m.isAlive).toBe(true); + expect(m.commands.totalExecuted).toBe(1); + expect(m.commands.historyLength).toBe(1); + expect(m.buffer.maxSize).toBe(1024); + expect(m.timeout.configuredSeconds).toBeNull(); + expect(m.timeout.isTimedOut).toBe(false); }); it("isIdleTimeout is false right after activity, true past the threshold", async () => { @@ -702,8 +702,8 @@ describe("InteractiveSession", () => { session.createdAt.setTime(Date.now() - 10_000); const m = await session.getDetailedMetrics(); - expect(m.timeout.configured_seconds).toBe(1); - expect(m.timeout.is_timed_out).toBe(true); + expect(m.timeout.configuredSeconds).toBe(1); + expect(m.timeout.isTimedOut).toBe(true); }); it("readOutput backfills execution_time and output_length on the last entry", async () => { @@ -712,8 +712,8 @@ describe("InteractiveSession", () => { await session.readOutput(100); const entry = session.commandHistory[0]!; - expect(entry.execution_time).toBeGreaterThanOrEqual(0); - expect(entry.output_length).toBeGreaterThan(0); + expect(entry.executionTime).toBeGreaterThanOrEqual(0); + expect(entry.outputLength).toBeGreaterThan(0); }); it("filterOutput returns matching lines (regex, case-insensitive)", async () => { diff --git a/typescript/__tests__/tools/interactive.test.ts b/typescript/__tests__/tools/interactive.test.ts index 4db43df..c28b55b 100644 --- a/typescript/__tests__/tools/interactive.test.ts +++ b/typescript/__tests__/tools/interactive.test.ts @@ -37,24 +37,24 @@ function makeSessionInfo(overrides: Partial = {}): Inter function makeMetrics(sessionId = "session-1"): SessionDetailedMetrics { return { - session_id: sessionId, + sessionId, state: SessionState.ACTIVE, - is_alive: true, - created_at: NOW, - last_activity: NOW, - uptime_seconds: 1, - idle_seconds: 0, - commands: { total_executed: 1, current_count: 1, history_length: 1 }, - performance: { total_cpu_time: 0.1, peak_memory_mb: 10, current_memory_mb: 8 }, - buffer: { current_size: 0, max_size: 1024, utilization_percent: 0 }, - timeout: { configured_seconds: null, is_timed_out: false }, + isAlive: true, + createdAt: NOW, + lastActivity: NOW, + uptimeSeconds: 1, + idleSeconds: 0, + commands: { totalExecuted: 1, currentCount: 1, historyLength: 1 }, + performance: { totalCpuTime: 0.1, peakMemoryMb: 10, currentMemoryMb: 8 }, + buffer: { currentSize: 0, maxSize: 1024, utilizationPercent: 0 }, + timeout: { configuredSeconds: null, isTimedOut: false }, }; } function makeManagerMetrics(): ManagerMetrics { return { - manager: { total_sessions: 1, active_sessions: 1, terminated_sessions: 0, max_sessions: 50, utilization_percent: 2 }, - aggregate: { total_commands: 5, total_cpu_time: 0.5, total_memory_mb: 8, avg_memory_per_session: 8 }, + manager: { totalSessions: 1, activeSessions: 1, terminatedSessions: 0, maxSessions: 50, utilizationPercent: 2 }, + aggregate: { totalCommands: 5, totalCpuTime: 0.5, totalMemoryMb: 8, avgMemoryPerSession: 8 }, sessions: [makeMetrics()], }; } @@ -345,7 +345,7 @@ describe("SessionHistoryTool", () => { it("returns session history", async () => { mgr.getSessionHistory.mockResolvedValue([ - { command: "help", timestamp: NOW, command_number: 1, execution_start: 0 }, + { command: "help", timestamp: NOW, commandNumber: 1, executionStart: 0 }, ]); const raw = await tool.execute("session-1"); const result = JSON.parse(raw); diff --git a/typescript/src/core/manager.ts b/typescript/src/core/manager.ts index 2ee93df..138fb17 100644 --- a/typescript/src/core/manager.ts +++ b/typescript/src/core/manager.ts @@ -176,9 +176,9 @@ export class OpenROADManager { try { const metrics = await session.getDetailedMetrics(); sessionDetails.push(metrics); - totalCommands += metrics.commands.total_executed; - totalCpuTime += metrics.performance.total_cpu_time; - totalMemoryMb += metrics.performance.current_memory_mb; + totalCommands += metrics.commands.totalExecuted; + totalCpuTime += metrics.performance.totalCpuTime; + totalMemoryMb += metrics.performance.currentMemoryMb; } catch (e) { this.logger.warn(`Failed to get metrics for session ${session.sessionId}: ${String(e)}`); } @@ -192,17 +192,17 @@ export class OpenROADManager { return { manager: { - total_sessions: totalSessions, - active_sessions: activeSessions, - terminated_sessions: terminatedSessions, - max_sessions: this.maxSessions, - utilization_percent: this.maxSessions > 0 ? (activeSessions / this.maxSessions) * 100 : 0, + totalSessions, + activeSessions, + terminatedSessions, + maxSessions: this.maxSessions, + utilizationPercent: this.maxSessions > 0 ? (activeSessions / this.maxSessions) * 100 : 0, }, aggregate: { - total_commands: totalCommands, - total_cpu_time: totalCpuTime, - total_memory_mb: totalMemoryMb, - avg_memory_per_session: activeSessions > 0 ? totalMemoryMb / activeSessions : 0, + totalCommands, + totalCpuTime, + totalMemoryMb, + avgMemoryPerSession: activeSessions > 0 ? totalMemoryMb / activeSessions : 0, }, sessions: sessionDetails, }; diff --git a/typescript/src/core/models.ts b/typescript/src/core/models.ts index 48d3420..378086e 100644 --- a/typescript/src/core/models.ts +++ b/typescript/src/core/models.ts @@ -39,64 +39,66 @@ export interface InteractiveExecResult { error?: string | null; } -// Opaque snake_case payloads -// These are passed straight through to the wire (no camel->snake conversion), -// matching Python's dict output byte-for-byte. +// Nested domain payloads (camelCase) +// Authored in camelCase like every other domain model and converted to the +// snake_case MCP wire format at the serialization boundary +// (BaseTool.formatResult -> toSnakeCase). Nothing here is hand-written in +// snake_case, so the wire convention is produced by a single rule. /** One entry in a session's command history. */ export interface CommandHistoryEntry { command: string; timestamp: string; - command_number: number; - execution_start: number; - execution_time?: number; - output_length?: number; + commandNumber: number; + executionStart: number; + executionTime?: number; + outputLength?: number; } /** Detailed per-session metrics returned by InteractiveSession.getDetailedMetrics. */ export interface SessionDetailedMetrics { - session_id: string; + sessionId: string; state: string; - is_alive: boolean; - created_at: string; - last_activity: string; - uptime_seconds: number; - idle_seconds: number; + isAlive: boolean; + createdAt: string; + lastActivity: string; + uptimeSeconds: number; + idleSeconds: number; commands: { - total_executed: number; - current_count: number; - history_length: number; + totalExecuted: number; + currentCount: number; + historyLength: number; }; performance: { - total_cpu_time: number; - peak_memory_mb: number; - current_memory_mb: number; + totalCpuTime: number; + peakMemoryMb: number; + currentMemoryMb: number; }; buffer: { - current_size: number; - max_size: number; - utilization_percent: number; + currentSize: number; + maxSize: number; + utilizationPercent: number; }; timeout: { - configured_seconds: number | null; - is_timed_out: boolean; + configuredSeconds: number | null; + isTimedOut: boolean; }; } /** Aggregate metrics across all sessions returned by OpenROADManager.sessionMetrics. */ export interface ManagerMetrics { manager: { - total_sessions: number; - active_sessions: number; - terminated_sessions: number; - max_sessions: number; - utilization_percent: number; + totalSessions: number; + activeSessions: number; + terminatedSessions: number; + maxSessions: number; + utilizationPercent: number; }; aggregate: { - total_commands: number; - total_cpu_time: number; - total_memory_mb: number; - avg_memory_per_session: number; + totalCommands: number; + totalCpuTime: number; + totalMemoryMb: number; + avgMemoryPerSession: number; }; sessions: SessionDetailedMetrics[]; } diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index 63c698a..f8f59cc 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -191,8 +191,8 @@ export class InteractiveSession { this.commandHistory.push({ command: command.trim(), timestamp: new Date().toISOString(), - command_number: this.commandCount + 1, - execution_start: Date.now() / 1000, + commandNumber: this.commandCount + 1, + executionStart: Date.now() / 1000, }); // Bound history so a long-lived session cannot grow it without limit. // command_number keeps increasing, so dropping the oldest entry is safe. @@ -419,9 +419,9 @@ export class InteractiveSession { private _recordReadResult(outputLength: number, executionTime: number): void { for (let i = this.commandHistory.length - 1; i >= 0; i--) { const entry = this.commandHistory[i]; - if (!entry || entry.execution_time !== undefined) break; - entry.execution_time = executionTime; - entry.output_length = outputLength; + if (!entry || entry.executionTime !== undefined) break; + entry.executionTime = executionTime; + entry.outputLength = outputLength; } this.lastActivity = new Date(); } @@ -460,31 +460,31 @@ export class InteractiveSession { const maxSize = this.outputBuffer.maxSize; return { - session_id: this.sessionId, + sessionId: this.sessionId, state: this._state, - is_alive: this.checkAlive(), - created_at: this.createdAt.toISOString(), - last_activity: this.lastActivity.toISOString(), - uptime_seconds: uptimeSeconds, - idle_seconds: idleSeconds, + isAlive: this.checkAlive(), + createdAt: this.createdAt.toISOString(), + lastActivity: this.lastActivity.toISOString(), + uptimeSeconds, + idleSeconds, commands: { - total_executed: this.totalCommandsExecuted, - current_count: this.commandCount, - history_length: this.commandHistory.length, + totalExecuted: this.totalCommandsExecuted, + currentCount: this.commandCount, + historyLength: this.commandHistory.length, }, performance: { - total_cpu_time: this.totalCpuTime, - peak_memory_mb: this.peakMemoryMb, - current_memory_mb: currentMemoryMb, + totalCpuTime: this.totalCpuTime, + peakMemoryMb: this.peakMemoryMb, + currentMemoryMb, }, buffer: { - current_size: bufferSize, - max_size: maxSize, - utilization_percent: maxSize > 0 ? (bufferSize / maxSize) * UTILIZATION_PERCENTAGE_BASE : 0, + currentSize: bufferSize, + maxSize, + utilizationPercent: maxSize > 0 ? (bufferSize / maxSize) * UTILIZATION_PERCENTAGE_BASE : 0, }, timeout: { - configured_seconds: this.sessionTimeoutSeconds, - is_timed_out: this._checkSessionTimeout(), + configuredSeconds: this.sessionTimeoutSeconds, + isTimedOut: this._checkSessionTimeout(), }, }; } @@ -511,7 +511,7 @@ export class InteractiveSession { async replayCommand(commandNumber: number): Promise { for (const cmd of this.commandHistory) { - if (cmd.command_number === commandNumber) { + if (cmd.commandNumber === commandNumber) { await this.sendCommand(cmd.command); return cmd.command; } From bf47001245240c22e5f72682dc43b0b8645f897c Mon Sep 17 00:00:00 2001 From: kartikloops Date: Mon, 29 Jun 2026 21:43:17 -0600 Subject: [PATCH 42/44] resolve tcl backslash escapes in the verb and bracket scan so an obfuscated command like \socket or \x73ocket is matched as the command tcl actually runs, not the literal backslash form --- .../config/command_whitelist.test.ts | 19 +++++ typescript/src/config/command_whitelist.ts | 69 +++++++++++++++++-- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/typescript/__tests__/config/command_whitelist.test.ts b/typescript/__tests__/config/command_whitelist.test.ts index 9063a70..d1bed9a 100644 --- a/typescript/__tests__/config/command_whitelist.test.ts +++ b/typescript/__tests__/config/command_whitelist.test.ts @@ -258,6 +258,14 @@ describe("isQueryCommand", () => { expect(isQueryCommand("report_wns }; exec ls")).toEqual([false, "exec"]); }); + it("blocks a backslash-escaped exec-only verb (\\glob -> glob)", () => { + expect(isQueryCommand("\\glob *")).toEqual([false, "glob"]); + }); + + it("blocks a backslash-escaped command in a bracket substitution", () => { + expect(isQueryCommand("puts [\\exec ls]")).toEqual([false, "exec"]); + }); + it("mid-word quote is literal and cannot hide a trailing exec", () => { expect(isQueryCommand('set x a"b ; exec ls')).toEqual([false, "exec"]); }); @@ -328,6 +336,17 @@ describe("isExecCommand", () => { it("unbalanced close brace cannot hide a trailing quit (depth clamp)", () => { expect(isExecCommand("set x } ; quit")).toEqual([false, "quit"]); }); + + it("blocks a backslash-escaped verb (\\socket -> socket)", () => { + expect(isExecCommand("\\socket localhost 1")).toEqual([false, "socket"]); + expect(isExecCommand("\\quit")).toEqual([false, "quit"]); + expect(isExecCommand("\\load /tmp/x")).toEqual([false, "load"]); + }); + + it("blocks a hex/octal-escaped verb (\\x73ocket / \\163ocket -> socket)", () => { + expect(isExecCommand("\\x73ocket localhost 1")).toEqual([false, "socket"]); + expect(isExecCommand("\\163ocket localhost 1")).toEqual([false, "socket"]); + }); }); // isCommandAllowed (backward-compat alias) diff --git a/typescript/src/config/command_whitelist.ts b/typescript/src/config/command_whitelist.ts index 01b3af9..72b0c34 100644 --- a/typescript/src/config/command_whitelist.ts +++ b/typescript/src/config/command_whitelist.ts @@ -146,20 +146,76 @@ function matchesAny(verb: string, matchers: readonly Minimatch[]): boolean { return matchers.some((m) => m.match(verb)); } +/** + * Resolve Tcl backslash substitution within a single command word so an + * obfuscated verb matches the command Tcl will actually run. Tcl drops a + * backslash before an ordinary character (`\socket` -> `socket`) and decodes + * numeric escapes (`\x73`/`\163`/`s` -> `s`), so without this a blocked + * verb could be smuggled past the allowlist as its literal-backslash form. A + * real command name never contains a backslash, so this only normalises + * obfuscation and never alters a legitimate verb. + */ +function tclUnescape(token: string): string { + if (!token.includes("\\")) return token; + let out = ""; + for (let i = 0; i < token.length; i++) { + if (token[i] !== "\\" || i + 1 >= token.length) { + out += token[i]; + continue; + } + const c = token[i + 1]!; + const simple: Record = { + a: "\x07", b: "\b", f: "\f", n: "\n", r: "\r", t: "\t", v: "\v", + }; + if (c in simple) { + out += simple[c]; + i++; + } else if (c === "x") { + const m = /^[0-9a-fA-F]+/.exec(token.slice(i + 2)); + if (m) { + out += String.fromCharCode(parseInt(m[0], 16) & 0xff); + i += 1 + m[0].length; + } else { + out += "x"; + i++; + } + } else if (c === "u" || c === "U") { + const m = new RegExp(`^[0-9a-fA-F]{1,${c === "u" ? 4 : 8}}`).exec(token.slice(i + 2)); + if (m) { + out += String.fromCodePoint(parseInt(m[0], 16)); + i += 1 + m[0].length; + } else { + out += c; + i++; + } + } else if (c >= "0" && c <= "7") { + const m = /^[0-7]{1,3}/.exec(token.slice(i + 1))!; + out += String.fromCharCode(parseInt(m[0], 8) & 0xff); + i += m[0].length; + } else { + // Backslash before an ordinary char: drop the backslash, keep the char. + out += c; + i++; + } + } + return out; +} + /** * Return the command verb (first token) of a single Tcl statement. * * Returns null only for blank lines and comment lines. Lines that start with a * substitution or grouping character (`$`, `[`, `]`, `{`, `}`) are returned - * as-is so the caller can reject them via the allowlist. + * as-is so the caller can reject them via the allowlist. Backslash escapes in + * the verb are resolved (see tclUnescape) so `\socket` is matched as `socket`. */ export function extractVerb(statement: string): string | null { const stripped = statement.trim(); if (stripped === "" || stripped.startsWith("#")) { return null; } - const firstToken = stripped.split(/\s+/)[0]!; - return firstToken.replace(/;+$/, ""); + const firstToken = stripped.split(/\s+/)[0]!.replace(/;+$/, ""); + return tclUnescape(firstToken); } // Unicode line-boundary characters that Python's str.splitlines() recognises. @@ -254,8 +310,11 @@ function* iterVerbs(command: string): Generator { const verb = extractVerb(stmt); if (verb === null) continue; yield verb; - for (const match of stmt.matchAll(/\[\s*(?::+)?(\w+)/g)) { - yield match[1]!; + // Capture word chars and backslash escapes after `[` so a backslash- + // obfuscated substituted command (`[\exec ls]`) is caught, then resolve + // the escapes to the verb Tcl runs. + for (const match of stmt.matchAll(/\[\s*(?::+)?((?:\\.|\w)+)/g)) { + yield tclUnescape(match[1]!).replace(/^:+/, ""); } } } From efdf7bcb383c943ae45594a7ec2c6caf86aebdba Mon Sep 17 00:00:00 2001 From: kartikloops Date: Mon, 29 Jun 2026 21:49:28 -0600 Subject: [PATCH 43/44] capture the bracket command word up to a delimiter so a dynamic substituted command ($x or nested [[...]]) is surfaced and rejected by the read-only tool instead of running exec via runtime variable substitution --- .../config/command_whitelist.test.ts | 12 +++++++++++ typescript/src/config/command_whitelist.ts | 21 ++++++++++++------- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/typescript/__tests__/config/command_whitelist.test.ts b/typescript/__tests__/config/command_whitelist.test.ts index d1bed9a..0b426c7 100644 --- a/typescript/__tests__/config/command_whitelist.test.ts +++ b/typescript/__tests__/config/command_whitelist.test.ts @@ -266,6 +266,18 @@ describe("isQueryCommand", () => { expect(isQueryCommand("puts [\\exec ls]")).toEqual([false, "exec"]); }); + it("blocks a variable-substituted command in a bracket ([$x] -> exec at runtime)", () => { + expect(isQueryCommand("set x exec; puts [$x ls]")).toEqual([false, "$x"]); + }); + + it("blocks a nested command substitution as the bracket command ([[...] ...])", () => { + expect(isQueryCommand("puts [[set x exec] ls]")).toEqual([false, "[set"]); + }); + + it("still allows a substituted argument that is a variable ([get_cells $x])", () => { + expect(isQueryCommand("puts [get_property [get_cells $x] area]")).toEqual([true, null]); + }); + it("mid-word quote is literal and cannot hide a trailing exec", () => { expect(isQueryCommand('set x a"b ; exec ls')).toEqual([false, "exec"]); }); diff --git a/typescript/src/config/command_whitelist.ts b/typescript/src/config/command_whitelist.ts index 72b0c34..3760b1f 100644 --- a/typescript/src/config/command_whitelist.ts +++ b/typescript/src/config/command_whitelist.ts @@ -295,9 +295,17 @@ function splitTclStatements(command: string): string[] { * ignored; `;` plus all Unicode line boundaries are separators so a \r cannot * hide a command), two kinds of verb are yielded: * 1. The statement verb — its leading token. - * 2. Bracket verbs — the word following each `[` (bracket substitution). This - * catches `set x [exec ls]` where the outer verb `set` is safe but the - * substituted command `exec` is not. + * 2. Bracket verbs — the command word following each `[` (bracket + * substitution). This catches `set x [exec ls]` where the outer verb `set` + * is safe but the substituted command `exec` is not. + * + * The bracket word is captured up to the first delimiter, so a substituted + * command whose name is itself dynamic is surfaced rather than skipped: a + * variable name (`[$x ls]`) yields `$x` and a nested substitution + * (`[[set x exec] ls]`) yields `[set`. Neither is in any allowlist, so the + * read-only tool rejects them instead of letting Tcl resolve `$x`->exec at + * runtime and run an arbitrary command. Backslash escapes are then resolved + * (see tclUnescape) so `[\exec ls]` is matched as `exec`. * * Comment and blank statements (extractVerb returns null) are skipped entirely, * including their brackets: a `#` comment is discarded at Tcl parse time and is @@ -310,10 +318,9 @@ function* iterVerbs(command: string): Generator { const verb = extractVerb(stmt); if (verb === null) continue; yield verb; - // Capture word chars and backslash escapes after `[` so a backslash- - // obfuscated substituted command (`[\exec ls]`) is caught, then resolve - // the escapes to the verb Tcl runs. - for (const match of stmt.matchAll(/\[\s*(?::+)?((?:\\.|\w)+)/g)) { + // `[` includes itself in the class so a nested `[[...]` substitution yields + // a `[`-led token; `$` is included so a `[$var]` indirection yields `$var`. + for (const match of stmt.matchAll(/\[\s*(?::+)?([^\s\]{};]+)/g)) { yield tclUnescape(match[1]!).replace(/^:+/, ""); } } From a58d30b2867cf34a55372dd7b7bc9052727df902 Mon Sep 17 00:00:00 2001 From: kartikloops Date: Mon, 29 Jun 2026 22:10:28 -0600 Subject: [PATCH 44/44] use function replacement in _detectErrors so $& or $1 in a captured error message is inserted literally instead of being reinterpreted as a replacement pattern --- typescript/__tests__/interactive/session.test.ts | 6 ++++++ typescript/src/interactive/session.ts | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/typescript/__tests__/interactive/session.test.ts b/typescript/__tests__/interactive/session.test.ts index 5a5a8c3..162d041 100644 --- a/typescript/__tests__/interactive/session.test.ts +++ b/typescript/__tests__/interactive/session.test.ts @@ -583,6 +583,12 @@ describe("InteractiveSession", () => { expect(result.error).toMatch(/Invalid command/); }); + it("inserts captured text literally even when it contains $& replacement patterns", async () => { + await session.outputBuffer.append('invalid command name "foo$&bar"\n'); + const result = await session.readOutput(100); + expect(result.error).toBe("Invalid command: foo$&bar"); + }); + it("returns null error for clean output", async () => { await session.outputBuffer.append("openroad> \n"); const result = await session.readOutput(100); diff --git a/typescript/src/interactive/session.ts b/typescript/src/interactive/session.ts index f8f59cc..62528fe 100644 --- a/typescript/src/interactive/session.ts +++ b/typescript/src/interactive/session.ts @@ -403,7 +403,9 @@ export class InteractiveSession { const match = pattern.exec(output); if (match) { const capture = match[1]; - return capture ? template.replace("{0}", capture.trim()) : template; + // 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; } }