@greptile please re-review this PR.

@greptile review

Greptile Summary

This PR fixes a bug where plugin before_reset handlers never fired when sessions were reset via the gateway path (TUI /new, /reset commands). The gateway sessions.reset handler now correctly calls hookRunner.runBeforeReset() before clearing the session.

Key implementation details:

• Eagerly reads transcript file (awaited) before archival can rename it, avoiding the race condition identified in previous iterations
• Dispatches hook execution fire-and-forget to avoid blocking the reset response
• Only fires after successful cleanup prechecks, matching the auto-reply path pattern
• Uses properly resolved target.agentId instead of naive string splitting

Testing:

• Comprehensive test coverage including hook firing, reason propagation, transcript parsing, and cleanup failure scenarios
• Test file properly creates real JSONL files and validates message extraction

Evolution:
The PR went through multiple iterations to get the design right, balancing race condition avoidance with response latency - the final implementation (585c5e3) splits eager file read from fire-and-forget dispatch.

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• Score reflects solid implementation with proper race condition handling, comprehensive test coverage (4 test cases covering happy path, edge cases, and failure scenarios), clear documentation, and backward compatibility. The fix addresses a real bug where plugins never received reset notifications from the dominant reset path in production. No security concerns, no breaking changes, and implementation follows established patterns from the auto-reply path.
• No files require special attention

_{Last reviewed commit: 585c5e3}

Fixed in 22c3fdc — transcript is now read eagerly (awaited) before the fire-and-forget runBeforeReset call. This eliminates the race between readFile and archiveSessionTranscriptsForSession. The hook invocation itself remains fire-and-forget via .catch().

@greptile please re-review this PR.
@codex review

Codex Review: Didn't find any major issues. Another round soon, please!

@greptile please re-review this PR.

Both Codex concerns addressed:

• P1: Hook now fires after prechecks succeed (9b2539f). Added test for cleanup failure → hook not called.
• P2: Transcript read is eager/awaited (22c3fdc). No race with archival.

@greptile please re-review this PR.
@codex review

Addressed P2 in a6456b6 — transcript read moved after cleanup completes.

Current flow: triggerInternalHook → cleanup prechecks → read transcript → fire hook (fire-and-forget) → updateSessionStore → archive

@greptile please re-review this PR.
@codex review

@codex review

@greptile please re-review this PR.

@greptile please re-review this PR.

Review note: Codex vs Greptile contradiction

The last two review cycles revealed a direct contradiction between the two bots:

• Greptile (#2868004236) flagged the fire-and-forget IIFE pattern (commit 4cecc1b) because archiveSessionTranscriptsForSession calls fs.renameSync on the transcript file after updateSessionStore, causing an ENOENT race with the async readFile.
• Codex (#2868008570) flagged the eager await readFile (commit 585c5e3) because it blocks the reset response path.

The current approach (585c5e3) is the correct trade-off:

1. Eager read (awaited): local file I/O is O(ms), and the read MUST happen before archival renames the file — otherwise messages are lost.
2. Fire-and-forget dispatch: only hookRunner.runBeforeReset() (the slow plugin RPC) is non-blocking via void ... .catch().

This is the only solution that avoids both the archival race AND keeps the slow hook dispatch off the response path.

@greptile @chatgpt-codex-connector please re-review this PR.

Codex Review: Didn't find any major issues. 🚀

🔒 Aisle Security Analysis

We found 2 potential security issue(s) in this PR:

1. 🟡 Resource exhaustion in sessions.reset before_reset hook due to unbounded transcript read/parse

Description

The sessions.reset handler reads the entire session transcript into memory and splits/parses it synchronously before sending the RPC response when a before_reset plugin hook is registered.

• sessionFile points to a JSONL transcript that can grow large in normal operation (long LLM conversations) and can also contain attacker-influenced content (user message volume/size).
• await fs.promises.readFile(sessionFile, "utf-8") loads the whole file into a single string (memory spike proportional to file size).
• content.split("\n") allocates an additional array of all lines (additional memory overhead) and then JSON.parse is executed for each line (CPU-intensive and blocks the event loop).
• messages.push(...) is unbounded, potentially accumulating a very large in-memory array.
• Despite the comment claiming “fire-and-forget … to avoid blocking the response”, the expensive read/split/parse happens prior to respond(...), so the request can be used to tie up the gateway process.

Vulnerable code:

const content = await fs.promises.readFile(sessionFile, "utf-8");
for (const line of content.split("\n")) {
  ...
  const parsed = JSON.parse(line);
  if (parsed.type === "message" && parsed.message) {
    messages.push(parsed.message);
  }
}

Recommendation

Avoid reading/splitting the entire transcript in the request path. Prefer streaming + hard limits (bytes/lines/messages) and consider truncating to the last N messages.

Example: stream JSONL lines with a maximum file size and message cap:

import { createReadStream } from "node:fs";
import readline from "node:readline";

const MAX_TRANSCRIPT_BYTES = 5 * 1024 * 1024; // tune
const MAX_MESSAGES = 200; // tune

const stat = await fs.promises.stat(sessionFile);
if (stat.size > MAX_TRANSCRIPT_BYTES) {
  ​// Either skip, or only read the tail, or pass a truncated indicator
}

const messages: unknown[] = [];
const rl = readline.createInterface({
  input: createReadStream(sessionFile, { encoding: "utf-8" }),
  crlfDelay: Infinity,
});

for await (const line of rl) {
  if (messages.length >= MAX_MESSAGES) break;
  if (!line.trim()) continue;
  try {
    const parsed = JSON.parse(line);
    if (parsed?.type === "message" && parsed.message) messages.push(parsed.message);
  } catch {
    ​// skip malformed lines
  }
}

Additionally:

• Cap messages by count and/or total JSON bytes (e.g., reuse capArrayByJsonBytes).
• If the hook must see the transcript even after archival, consider archiving first and passing the archived path, then parsing asynchronously in a background task (so respond(...) is not delayed).

2. 🔵 Arbitrary file read via unvalidated sessionFile path in sessions.reset before_reset hook

Description

In sessions.reset, the new before_reset hook implementation reads entry.sessionFile directly from the session store and passes parsed content to plugin hooks.

• entry.sessionFile is loaded from the persisted sessions store (loadSessionEntry(key) → sessions.json). If this stored path is stale/corrupt or attacker-tampered (e.g., via sessions store manipulation, untrusted state sync/restore, or any bug that allows writing to the store), the gateway will read an arbitrary file path on disk.
• The read uses fs.promises.readFile(sessionFile, "utf-8") with no containment check, no realpath/symlink defense, and no size limit.
• The file contents are then forwarded to plugin code via hookRunner.runBeforeReset(...), which can exfiltrate sensitive host data.

Vulnerable code:

const sessionFile = entry?.sessionFile;
...
const content = await fs.promises.readFile(sessionFile, "utf-8");

Recommendation

Treat entry.sessionFile as untrusted metadata and only read transcripts through a path-resolution + boundary check.

Recommended changes:

1. Resolve a safe transcript path using the existing session transcript helpers (these perform containment checks and realpath-based handling for legacy absolute paths):

import path from "node:path";
import { resolveSessionFilePath } from "../../config/sessions/paths.js";

const sessionsDir = path.dirname(storePath);
const safeTranscriptPath = entry?.sessionId
  ? resolveSessionFilePath(entry.sessionId, { sessionFile }, { sessionsDir, agentId: target.agentId })
  : null;

2. Enforce boundary + size limits (and avoid following symlinks outside the sessions dir) by opening the file with the existing boundary-safe helper (or equivalent) and reading only up to a maximum size (or streaming JSONL line-by-line):

import { openBoundaryFile } from "../../infra/boundary-file-read.js";

const opened = await openBoundaryFile({
  absolutePath: safeTranscriptPath,
  rootPath: sessionsDir,
  boundaryLabel: "sessions dir",
  maxBytes: 2 * 1024 * 1024, // example cap
});
if (!opened.ok) {
  ​// fire hook with empty messages
} else {
  try {
    const content = fs.readFileSync(opened.fd, "utf-8");
    ​// parse jsonl...
  } finally {
    fs.closeSync(opened.fd);
  }
}

3. Consider capping messages (count and/or total bytes) passed to plugins to prevent memory/CPU spikes on large transcripts.

Analyzed PR: #29969 at commit 65fcaf3

_{Last updated on: 2026-03-05T04:29:59Z}

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.