Skip to content

feat: Terminal - #14

Merged
juliusmarminge merged 3 commits into
mainfrom
feat/terminal
Feb 12, 2026
Merged

feat: Terminal#14
juliusmarminge merged 3 commits into
mainfrom
feat/terminal

Fix ChatView height scroll

42b2a88
Select commit
Loading
Failed to load commit list.
MacroscopeApp / Macroscope - Correctness Check completed Feb 12, 2026 in 7m 24s

1 issue identified (110 code objects reviewed).

• Merge Base: 16b81f7
• Head: 42b2a88

Details

✅ File Path Comments Posted
✅ apps/server/src/git.ts 0
✅ apps/server/src/ptyAdapter.ts 0
❌ apps/server/src/terminalManager.ts 1
✅ apps/server/src/wsServer.ts 0
✅ apps/web/src/App.tsx 0
✅ apps/web/src/components/ChatView.tsx 0
✅ apps/web/src/components/Sidebar.tsx 0
✅ apps/web/src/components/ThreadTerminalDrawer.tsx 0
✅ apps/web/src/index.css 0
✅ apps/web/src/persistenceSchema.ts 0
✅ apps/web/src/store.ts 0
✅ apps/web/src/terminal-shortcuts.ts 0
✅ apps/web/src/types.ts 0
✅ apps/web/src/wsNativeApi.ts 0
✅ packages/contracts/src/ipc.ts 0
✅ packages/contracts/src/terminal.ts 0

Filtered Issues Details

apps/server/src/git.ts
  • line 50: When maxBytes is reached in runGit or runTerminalCommand, the truncation logic continues to append chunk.subarray(0, remaining) even when remaining is zero or negative because the negative check is remaining <= 0, but the subsequent subarray(0, remaining) call with a negative remaining (if remaining was exactly 0, it's fine, but if it was negative, subarray behaves differently). Actually, looking closer: remaining is maxBytes - currentBytes. If remaining <= 0 (e.g. currentBytes >= maxBytes), it returns early (lines 45-47). This logic seems sound for the negative case. [ Already posted ]
  • line 50: The logic in appendChunkWithinLimit uses .toString() on chunk (line 50) or a subarray of chunk (line 56). This assumes the default encoding (utf8). If the command outputs binary data or uses a different encoding that is invalid in UTF-8, this will insert replacement characters or garbage into the stdout/stderr string. Furthermore, the stdoutBytes and stderrBytes counters track the length of the string (or accumulated bytes so far) plus chunk.length. However, nextBytes is calculated as currentBytes + chunk.length (line 51) or currentBytes + remaining (line 57). This tracks byte count. But stdout is a JS string. Later, if maxOutputBytes is large, the string length (in UTF-16 code units) might diverge significantly from byte length if multi-byte characters are present. Mixing byte-length limits with string concatenation is generally safe for limits, but relying on chunk.toString() inside the chunk handler without a StringDecoder guarantees corruption of multi-byte characters that span chunk boundaries. [ Already posted ]
  • line 88: The updated runGit function enforces a new hardcoded 1MB limit on stdout. If command output exceeds this (e.g., git branch on a large repo), stdout is truncated and a warning is appended to stderr, but the process still exits with code: 0. The caller listGitBranches ignores stderr when code is 0 and proceeds to parse the truncated stdout. This results in listGitBranches silently returning an incomplete list of branches, and potentially a corrupted final branch name if the truncation occurs mid-string, with no runtime error indicating data loss. [ Already posted ]
apps/server/src/terminalManager.ts
  • line 60: The capHistory function can create infinite string growth or memory pressure if the input history string is large and does not contain newlines, because history.split("\n") will create a single-element array (length 1) which is always <= maxLines (unless maxLines is 0), returning the original massive string without capping. While maxLines defaults to 5,000, if onProcessData receives a stream of data without newlines (e.g., a progress bar or binary output), session.history will grow unbounded in memory until the process crashes. [ Already posted ]
  • line 72: The newly introduced legacySafeThreadId function sanitizes inputs using an allowlist regex /[^a-zA-Z0-9._-]/g but fails to filter Windows reserved device names (e.g., CON, PRN, AUX, NUL, COM1). When readHistory calls this function via legacyHistoryPath, it may construct a path like .../CON.log. On Windows, fs.promises.readFile treats this path as the console device driver. If the process is attached to a console, the read operation will block indefinitely waiting for input. An adversary can trigger this via open({ threadId: "CON" }), causing the request to hang. Furthermore, concurrent requests can exhaust the libuv thread pool, leading to a denial of service for the entire application. [ Already posted ]
  • line 254: The dispose method clears this.threadLocks without waiting for or cancelling pending operations, breaking the mutual exclusion guarantee of runWithThreadLock. If dispose() is called while a close() operation is suspended (e.g., awaiting flushPersistQueue), the lock for that thread ID is effectively released. A subsequent open() call for the same thread ID can then immediately acquire the lock and create a new session. When the suspended close() operation eventually resumes, it proceeds to call deleteHistory (if requested), which deletes the history file that the newly created session may have just loaded or initialized. This results in data loss (persisted history removal) for an active session. [ Already posted ]
  • line 309: Memory leak in startSession error handler due to incomplete cleanup. If an exception occurs in startSession after the listeners are attached but before the function completes (e.g., if this.emitEvent at line 293 throws because of a failing event listener), the catch block cleans up the process state (kills process, sets session.process to null) but fails to remove the event listeners by calling session.unsubscribeData or session.unsubscribeExit. This leaves the onData listener attached to the underlying node-pty object. Since the listener closure captures the session object (line 287), and the session object is retained in this.sessions, a circular reference prevents the node-pty process instance (and its associated buffers/resources) from being garbage collected until the session is manually closed or successfully restarted. [ Already posted ]
  • line 326: The capHistory function limits history by line count but fails to limit line length, leading to unbounded memory growth. In onProcessData, incoming data is appended to session.history and then passed to capHistory. If a process outputs a large stream of data without newlines (e.g., printing a binary file or using cat /dev/urandom), capHistory splits the string but finds only 1 line, bypassing the maxLines check. This causes session.history to grow until the Node.js process crashes with an Out Of Memory (OOM) error. [ Already posted ]
  • line 439: The readHistory method reads the entire content of the history file into memory using fs.promises.readFile (line 439) and subsequently processes it with capHistory (line 440), which splits the string by newlines. If a legacy history file is significantly large (e.g., generated by a previous version without limits or by an attacker), this operation will consume excessive memory (creating millions of string objects), creating a high risk of an Out-Of-Memory (OOM) crash and Denial of Service for the server process. [ Already posted ]
  • line 446: The readHistory method performs both readFile and writeFile operations within the same try block (lines 438-444). If readFile succeeds (loading the history) but the subsequent writeFile (line 442) fails with an ENOENT error (e.g., if the logs directory is deleted concurrently or permissions change such that the directory appears missing), the catch block (line 446) incorrectly interprets this as "file not found". It then swallows the error and falls through to the legacy file check. If the legacy file is also missing, the method returns an empty string (line 460), resulting in the silent loss of the history data that was successfully read from the new path. [ Already posted ]
apps/server/src/wsServer.ts
  • line 88: In runGit inside apps/server/src/git.ts, the new logic uses a helper function appendChunkWithinLimit which is not imported or defined in the file. The diff shows lines removing stdout += chunk.toString() and replacing them with calls to appendChunkWithinLimit, but there is no import or definition of this function in the provided git.ts file content. This will cause a ReferenceError: appendChunkWithinLimit is not defined at runtime whenever runGit processes output. [ Already posted ]
apps/web/src/components/ThreadTerminalDrawer.tsx
  • line 305: The inputDisposable callback in useEffect captures the terminal instance and attempts to write to it in a .catch() block after an asynchronous operation. If the component unmounts while the api.terminal.write promise is pending, the useEffect cleanup will run terminal.dispose(). When the promise subsequently rejects (e.g. due to the unmount closing the connection), the catch block calls writeSystemMessage which executes terminal.write(...) on the disposed terminal instance. Accessing methods on a disposed xterm.js instance throws a runtime error (typically TypeError on internal _core). [ Already posted ]
  • line 336: The openTerminal function (line 323) is async and called via void openTerminal() inside a useEffect. If the effect cleanup runs (line 391) before openTerminal awaits api.terminal.open, the disposed flag is set to true. However, openTerminal checks disposed (line 335) after the await. If terminal.dispose() (line 399) has already run in the cleanup, terminalRef.current is set to null (line 397). But openTerminal holds a local reference activeTerminal captured at the start (line 325). The dispose() call on the terminal instance destroys it. Subsequent calls like activeTerminal.write (line 336, 338) or activeTerminal.focus (line 341) on a disposed terminal instance will throw a runtime error (xterm.js throws when interacting with a disposed instance). [ Already posted ]
packages/contracts/src/terminal.ts
  • line 41: The terminalSessionSnapshotSchema defines the history field as an unbounded z.string(). In a terminal application, output accumulates indefinitely. Consequently, for any long-running or verbose session (e.g., one running a build process or producing large logs), the history string can grow to hundreds of megabytes. Attempting to validate, serialize, or transmit a snapshot with such a large string (e.g., in terminalRestartedEventSchema) creates a high risk of Node.js process crashes due to Out-Of-Memory (OOM) errors or exceeding V8's maximum string length limits. [ Already posted ]
View more details on MacroscopeApp