fix(coding-agent): tail appended session JSONL in large-session TUI#3281
fix(coding-agent): tail appended session JSONL in large-session TUI#3281wolfiesch wants to merge 2 commits into
Conversation
Stop full transcript rebuilds on every poll in AgentTranscriptViewer by tailing appended JSONL bytes, buffering partial trailing lines, and only doing a compaction-aware full rebuild on file identity change, truncation, or structural entries. Collapse compacted display history for live chat rebuilds, and make FileSessionStorage.writeTextSync replace files via temp-write + rename so POSIX identity changes are observable. Refs can1357#3258
wolfiesch
force-pushed
the
wolfiesch/optimize
branch
from
082ae7e to
e3fca3e
Compare
github-actions
Bot
added
vouched
There was a problem hiding this comment.
Pull request overview
This PR improves large-session TUI performance in packages/coding-agent/ by switching parked-agent/advisor transcript rendering from full transcript rebuilds on every poll tick to incremental JSONL tailing, and by adding a display-only option to collapse pre-compaction history for hot transcript surfaces.
Changes:
- Implement append-only tailing for local and remote session JSONL in
AgentTranscriptViewer, with fallback rebuilds on rotation/rewrite/truncation and partial-line buffering. - Add
collapseCompactedHistory(display-only) tobuildSessionContext, and thread it through live transcript call sites. - Add/extend tests covering tailing behavior, rotation handling, collapsed transcript context, and POSIX inode replacement on sync rewrites.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/coding-agent/src/modes/components/agent-transcript-viewer.ts | Implements local/remote JSONL tailing, partial-line buffering, and conditional rebuilds. |
| packages/coding-agent/src/modes/components/chat-transcript-builder.ts | Adds append/rebuild-by-messages APIs to reuse the existing per-message render path. |
| packages/coding-agent/src/session/session-context.ts | Introduces collapseCompactedHistory for display transcripts and preserves OpenAI remote compaction payloads. |
| packages/coding-agent/src/session/agent-session.ts | Threads transcript collapse option through buildTranscriptSessionContext. |
| packages/coding-agent/src/modes/utils/ui-helpers.ts | Requests collapsed display transcripts for initial message rendering. |
| packages/coding-agent/src/modes/interactive-mode.ts | Uses collapsed transcript context for live transcript rebuild display. |
| packages/coding-agent/src/session/session-storage.ts | Changes sync session rewrites to temp+rename (inode replacement) with EPERM fallback. |
| packages/coding-agent/test/agent-hub-advisor-scroll.test.ts | Adds tests for local tailing, partial-line buffering, remote placeholder clearing, and rotation behavior. |
| packages/coding-agent/test/session-manager/build-context.test.ts | Adds tests for collapsed display transcript behavior and preserved provider payloads. |
| packages/coding-agent/test/modes/utils/render-initial-messages.test.ts | Updates transcript context spy signature and asserts collapse option is used. |
| packages/coding-agent/test/session-storage.test.ts | Adds POSIX inode identity replacement assertion for sync rewrites. |
| packages/coding-agent/CHANGELOG.md | Adds a user-facing entry describing the performance fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fd = fs.openSync(sessionFile, "r"); | ||
| const length = stat.size - state.offset; | ||
| const buffer = Buffer.allocUnsafe(length); | ||
| fs.readSync(fd, buffer, 0, length, state.offset); | ||
| const { complete, pending } = splitCompleteJsonl(state.pending + buffer.toString("utf-8")); | ||
| const entries = complete ? parseSessionEntries(complete) : []; | ||
| this.#localState = { | ||
| ...state, | ||
| size: stat.size, | ||
| mtimeMs: stat.mtimeMs, | ||
| ctimeMs: stat.ctimeMs, | ||
| offset: stat.size, | ||
| pending, | ||
| }; |
| /** | ||
| * Build the full-history display transcript instead of the LLM context: | ||
| * every path entry in chronological order, with each compaction emitted | ||
| * inline as a `compactionSummary` message at the position it fired rather | ||
| * than replacing the history before it. Display-only — never send the | ||
| * result to a provider. | ||
| */ | ||
| transcript?: boolean; | ||
| /** | ||
| * Display-only transcript optimization honored only when `transcript: true`: | ||
| * collapse history before the latest compaction while keeping display-visible | ||
| * turns from `firstKeptEntryId` onward. | ||
| */ | ||
| collapseCompactedHistory?: boolean; |
| if (options.collapseCompactedHistory === true && compaction) { | ||
| handleEntryResetTracking(compaction); | ||
| pushCompactionSummary(compaction); | ||
| const compactionIdx = path.findIndex(e => e.type === "compaction" && e.id === compaction.id); | ||
| let foundFirstKept = false; | ||
| for (let i = 0; i < compactionIdx; i++) { | ||
| const entry = path[i]; | ||
| if (entry.id === compaction.firstKeptEntryId) { | ||
| foundFirstKept = true; | ||
| } | ||
| if (foundFirstKept) { | ||
| appendMessage(entry); | ||
| } | ||
| } | ||
| for (let i = compactionIdx + 1; i < path.length; i++) { | ||
| appendMessage(path[i]); | ||
| } | ||
| } else { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3fca3e8ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #messagesFromEntries(entries: readonly FileEntry[]): AgentMessage[] { | ||
| const sessionEntries = entries.filter(isSessionEntry); | ||
| return buildSessionContext(sessionEntries, undefined, undefined, { | ||
| transcript: true, | ||
| collapseCompactedHistory: true, | ||
| }).messages; |
There was a problem hiding this comment.
When Agent Hub opens a legacy transcript file (header has no version or version < 2), parseSessionEntries() returns entries without the id/parentId tree fields, but this new full-rebuild path feeds them directly into buildSessionContext() instead of running the normal session migration first. In that case buildSessionContext() treats the last entry as the leaf and can only build a one-entry path, so initial loads, file replacements, or structural-entry rebuilds show only the final row instead of the full parked/advisor transcript; other read-only session loaders call migrateToCurrentVersion() before building context.
Useful? React with 👍 / 👎.
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
P3: scoped to #3258, but it duplicates the already-open maintainer fix PR #3259, so I would deprioritize this unless #3259 is abandoned.
Findings: changelog entry lands in released 16.1.15, and model-only transcript tail chunks update #model without scheduling a render.
Verification: bun --cwd=packages/coding-agent run check passed; focused bun test runs were blocked by missing packages/coding-agent/src/export/html/tool-views.generated.js in this worktree.
Thanks for tackling the large-session transcript path.
| - Fixed configured model discovery caches to refresh when `models.yml`/`models.json` is newer than the cached row, so updated local model metadata is not shadowed by fresh `models.db` entries. ([#3242](https://github.com/can1357/oh-my-pi/issues/3242)) | ||
| - Fixed hide-secrets handling so advisor session updates are redacted before the advisor model sees them and opaque assistant thinking blocks are no longer deobfuscated. | ||
| - Filtered alias definitions brush's whitespace-only expander cannot execute (`(`, `)`, `|`, `&`, `;`, `<`, `>`, `` ` ``) from the bash-tool shell snapshot, so user rc-files containing compound aliases like Fedora's default `which='(alias; declare -f) | /usr/bin/which …'` no longer poison the brush session with `error: command not found: (alias;` ([#3234](https://github.com/can1357/oh-my-pi/issues/3234)). | ||
| ### Fixed |
There was a problem hiding this comment.
should-fix: This adds a second ### Fixed inside the already-released ## [16.1.15] section. Repo convention requires new entries under ## [Unreleased] and released sections are immutable; this entry should be moved up to the existing Unreleased ### Fixed block at lines 9-15.
| } else if (entry.type === "model_change") { | ||
| this.#model = entry.model; |
There was a problem hiding this comment.
should-fix: A model_change append mutates #model here but returns an empty messages array, so the callers at #refresh()/#fetchRemote() go through #appendMessages([]) and never call requestRender() (except the remote first-fetch special case). The header model badge at #headerLines() stays stale until a later message arrives; either treat model-only chunks as a render-worthy incremental update or make the caller request a render when #incrementalMessages() consumed only metadata.
CI note: failing
|
…pt viewer The append-tailing refactor routed full local/remote rebuilds through buildSessionContext(transcript:true), which strips dangling (resultless) tool_use blocks for LLM-context safety. That hid a parked/advisor agent's in-flight tool call (e.g. a pending search) from the hub transcript. Display semantics differ from LLM semantics: only collapse through buildSessionContext when a compaction is present; otherwise map message entries verbatim so pending calls render, matching prior viewer behavior.
wolfiesch
force-pushed
the
wolfiesch/optimize
branch
from
2730c69 to
8459ac7
Compare
Update: fixed a real regression the CI surfaced (plus the Julia flake note above)Re-running CI shook out a genuine regression in this PR — separate from the
Fix ( Verification (all green locally): History cleaned: dropped the two empty |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8459ac71ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fs.readSync(fd, buffer, 0, length, state.offset); | ||
| const { complete, pending } = splitCompleteJsonl(state.pending + buffer.toString("utf-8")); |
There was a problem hiding this comment.
When the transcript is replaced or truncated between statSync and readSync (now plausible because rewrites are rename-based), readSync can return fewer bytes than the requested length; decoding the entire Buffer.allocUnsafe(length) then feeds uninitialized bytes into JSONL splitting/parsing and can render bogus rows or advance the tail past unread data. Capture the returned byte count and decode only buffer.subarray(0, bytesRead).
Useful? React with 👍 / 👎.
| return buildSessionContext(sessionEntries, undefined, undefined, { | ||
| transcript: true, | ||
| collapseCompactedHistory: true, | ||
| }).messages; |
There was a problem hiding this comment.
For any transcript that already contains a compaction, this full-rebuild path routes the display through buildSessionContext, whose final cleanup strips assistant toolCall blocks without matching toolResults. If a compacted advisor/parked agent is opened or its file is replaced while it is currently between a tool call and result, the pending tool spinner/assistant turn disappears until the result is written, unlike the non-compacted path that maps messages verbatim. The display collapse needs to preserve dangling tool calls for transcript viewers.
Useful? React with 👍 / 👎.
What
Stop the large-session TUI from rebuilding the entire transcript on every poll tick.
AgentTranscriptViewernow tails appended session JSONL and appends only the new rows, falling back to a compaction-aware full rebuild only when it must:dev/ino), size, mtime/ctime, byte offset, and a partial-line buffer. Each poll reads only[offset, size), parses complete newline-terminated lines, buffers an incomplete trailing line, and appends. A full rebuild fires only on identity change, truncation (size < offset), or a structural (non-message) entry.AgentHubRemote.readTranscript(id, fromByte)chunks, clear stale rows on host rotation (newSize < fromByte), and clear theLoading transcript from host…placeholder on a metadata-only first fetch.collapseCompactedHistoryoption tobuildSessionContext; live chat rebuilds (interactive-mode,ui-helpers) collapse pre-compaction history while still showing the kept turns. The OpenAI remotereplacementHistoryprovider payload is preserved for replay; only the display path collapses.FileSessionStorage.writeTextSyncnow writes a temp file and renames over the target (with the existing EPERM move-aside fallback, done synchronously), so the synchronous session rewrite path changes inode on POSIX and the viewer treats same-inode growth as a true append.ChatTranscriptBuildergainsappend/appendMessages/rebuildMessagesthat reuse the existing#appendChatMessagerender path — no second rendering path is introduced.Why
Fixes #3258. On long sessions, the parked-agent/advisor transcript viewer re-read, re-parsed, and rebuilt every row on every ~250 ms poll, and live chat rebuilds replayed full compacted history. Both scale with session length and stall the hot path.
Local A/B benchmark (machine-relative, generated fixtures)
Same untracked script run on clean
origin/mainvs this branch; 5,000 turns / 10,000 message entries. The script is local-only and not in the diff.The viewer per-append path runs every poll tick on every append; collapse savings scale with pre-compaction history length and are modest on this append-dominated workload. Microbenchmark of the builder + context hot paths, not an end-to-end frame-timing measurement.
Testing
All pass. Tests were written test-first and cover: local append tailing, partial-line buffering, full rebuild on file replacement/truncation, remote rotation + metadata-only placeholder clearing, collapsed transcript context (including OpenAI remote kept-turn display + preserved provider payload), the live chat callsite options, and sync-rewrite inode replacement (POSIX).
bun checkpasses