fix(sessions): page history by visible-message yield, not raw event count (lr-c24b)#306
Conversation
…24b) Add shared isVisibleHistoryEvent predicate (CJS + ESM copies, mirroring the lr-336f model-context-windows parity pattern) that classifies a recorded history event as visible vs. invisible-yield, matching the render dispatch in app-messages.js. Wire it into sessions.js: findLastTurnStart's initial-replay window and project-sessions.js's load_more_history handler now extend the page window backward turn-boundary by turn-boundary (bounded at MAX_VISIBILITY_EXTENSIONS steps, each itself capped at HISTORY_PAGE_SIZE per the existing findTurnBoundary guard) until the slice contains at least one visibly-rendering event or from reaches 0.
…(lr-c24b) prependOlderHistory: snapshot the DOM sibling immediately before the prepend anchor before rendering a batch. If the batch inserted nothing new (all-invisible-yield page) and more history remains, immediately request the next page instead of settling loadingMore:false — one click must always surface >=1 visible message. updateHistorySentinel: never render a solitary "Load earlier messages" button with no visible content below it. Skipped during an in-progress initial replay (replayingHistory), where an empty #messages container is expected transiently, not stuck. Adds regression coverage: history-visibility-parity.test.js (CJS/ESM predicate parity, mirroring the lr-336f model-context-windows pattern), history-pagination-visible-yield-lr-c24b.test.js (server-side bounded window extension), history-pagination-client-auto-advance-lr-c24b.test.js (client auto-advance + sentinel invariant, source-text regression style per the diagnostics.js DOM-test convention — no jsdom in this repo). npm test: 683/683 passing.
There was a problem hiding this comment.
PEACHES — clean (0 findings)
Reviewed:
- lib/history-visibility.js (CJS backend predicate, 113 lines)
- lib/public/modules/history-visibility.js (ESM frontend predicate, 98 lines)
- lib/sessions.js (extendWindowForVisibility, sliceHasVisibleEvent, 65 lines)
- lib/public/modules/app-header.js (updateHistorySentinel sentinel guard, prependOlderHistory auto-advance, sendLoadMoreHistory helper, 110 lines)
- lib/project-sessions.js (load_more_history call to extendWindowForVisibility, 7 lines)
- test/history-visibility-parity.test.js (78 lines, parity enforcement)
- test/history-pagination-visible-yield-lr-c24b.test.js (150 lines, server-side logic + zero-yield regression)
- test/history-pagination-client-auto-advance-lr-c24b.test.js (112 lines, client-side logic regression)
Findings verified:
-
Shared predicate parity (rule 1 — scope check): CJS and ESM copies of HIDDEN_TOOL_NAMES and INVISIBLE_TYPES are byte-equivalent (diff lines 45–87 vs 303–345). Parity test (test/history-visibility-parity.test.js) asserts both versions produce identical classifications for 26 representative event types. ✓
-
Server page-window extension bounded (rule 2 — no unbounded scan): extendWindowForVisibility(history, from, to) at lib/sessions.js:532–541 is capped at MAX_VISIBILITY_EXTENSIONS=5 steps (line 485). Each step via findTurnBoundary scans at most HISTORY_PAGE_SIZE=100 events backward (line 508). Worst-case backward scan: 5 × 100 = 500 events. Bounded. Regression test (test/history-pagination-visible-yield-lr-c24b.test.js lines 679–709) verifies the extension cap is enforced on an all-invisible session. ✓
-
Client auto-advance bounded, no infinite loop (rule 3 — cycle prevention): prependOlderHistory (lib/public/modules/app-header.js:286–289) auto-advances only when renderedNoVisibleContent && meta.hasMore. When meta.hasMore=false (server-side check indicating history exhausted), auto-advance branch is never taken — loadingMore is settled to false (line 291). Each response moves historyFrom backward (meta.from decreases per server's extendWindowForVisibility progress), guaranteeing forward progress across the iteration. ✓
-
Client sentinel-never-alone invariant (rule 3 — sentinel logic): updateHistorySentinel (lib/public/modules/app-header.js:128–170) detects isEmpty via hasRenderedContent = !!(existing ? existing.nextElementSibling : messagesEl.firstElementChild) (line 141). If empty and not mid-replay (replayingHistory check, line 142), triggers requestMoreHistory() instead of rendering a solitary button (line 145). Exception carved for initial-replay (replayingHistory=true) where empty #messages is transient and expected. Regression test (test/history-pagination-client-auto-advance-lr-c24b.test.js lines 554–579) verifies both the detection and the exception. ✓
-
Rendering snapshot logic sound (rule 4 — visibility detection): prependOlderHistory snapshots anchorEl.previousSibling before rendering the batch (line 231), then checks if it changed after all processMessage() calls (lines 247–249). addToMessages() (app-rendering.js:182–190) is the single insertion choke point — always insertBefore(el, prependAnchor) when prepending. Snapshot detects whether ANY top-level DOM was inserted before the anchor. ✓
-
Zero-yield-page regression coverage (amos.code-craft.4):
- test/history-pagination-visible-yield-lr-c24b.test.js lines 657–677: page landing entirely on invisible-yield events extends backward to find a visible turn.
- lines 723–730: a 100-event page of TodoWrite/TaskCreate/plan-tool bookkeeping is classified fully invisible.
- lines 732–735: a single Bash tool_start amid 99 invisible events makes the page visible.
- These directly exercise isVisibleHistoryEvent classification on the exact reproduction shape (all-invisible + one visible element) from the bug description. ✓
-
Defensive type defaults: isVisibleHistoryEvent returns true for null, missing-type, or unknown types (lib/history-visibility.js lines 106–107, return true fallthrough at line 120). Safe — can never under-count visible events, only possibly over-count. Comments explain rationale (lines 101–104). ✓
No violations of amos.code-craft, amos.path-choice, or project rules.
There was a problem hiding this comment.
BOBBIE — clean
Security audit of PR #306 (fix/lr-c24b-history-pagination-visible-yield), scope: server-side page-window extension in lib/sessions.js / lib/project-sessions.js, client auto-advance + sentinel guard in lib/public/modules/app-header.js, and the new shared history-visibility predicate (lib/history-visibility.js CJS + lib/public/modules/history-visibility.js ESM).
Findings: none blocking.
Audit notes on the flagged surface (load_more_history handler, lib/project-sessions.js:118-139):
- msg.before and msg.target are client-controlled and unclamped on the upper bound, but the handler does not expose a resource-exhaustion path. findTurnBoundary and extendWindowForVisibility (lib/sessions.js:507-541) are both internally bounded (searchFloor = targetIndex - HISTORY_PAGE_SIZE, and a hard MAX_VISIBILITY_EXTENSIONS=5 step cap respectively) regardless of how large or malformed before/target are.
- session.history.slice(from, to) at lib/project-sessions.js:132 relies on native Array.prototype.slice index clamping — an out-of-range or huge
beforeclamps to history.length / returns an empty array rather than performing unbounded work. - hydrateImageRefs (lib/project-image.js, unmodified by this PR) only transforms fields already present on server-recorded session.history entries (imageRefs, tab.screenshotFile) into URL strings; it does not read from disk or take a client-controlled path segment, so the sliced items reaching it do not introduce a new sink.
- No new dependency declared in this diff; package-lock.json is untouched by PR #306 (pre-existing repo-wide osv-scanner findings are out of scope for this PR).
Observed but not cited as a finding: lib/history-visibility.js / lib/public/modules/history-visibility.js index INVISIBLE_TYPES / HIDDEN_TOOL_NAMES plain object literals by entry.type / entry.name without an Object.create(null) or hasOwnProperty guard, so a maliciously-typed entry.type of "proto" or "constructor" would read the object prototype rather than undefined. Impact is limited to client-side visibility bookkeeping (whether a page is treated as having rendered content), entries originate from server-recorded session history rather than directly from the load_more_history message body, and no rulebook rule (bobbie.sast.1-7, bobbie.secret., bobbie.dep., bobbie.bleed.*) cites this pattern by name — dropped per "no citable rule, drop it."
Scanners run: semgrep (p/javascript, 68 rules, 0 findings across the 5 modified/added lib files), gitleaks (repo-wide, no-git filesystem scan — 6 hits, all in gitignored .claude/worktrees/*/lib/certs/privkey.pem local dev-cert scratch files, not tracked by git and not part of this PR's diff), osv-scanner (repo package-lock.json — 46 pre-existing vulnerabilities across 17 packages, none introduced by this PR, package-lock.json not touched by this diff).
scanners_run: semgrep(config=p/javascript, ok, 0 findings), gitleaks(no-git fs scan, ok, 6 hits all out-of-scope untracked worktree certs), osv-scanner(package-lock.json, ok, 46 pre-existing findings, no new deps in diff)
early_exit_reason: not applicable — full reasoning pass completed over the flagged handler and shared predicate module; zero blocking findings.
audit_scope.head_sha: 546bc16 (local repo HEAD at audit time; PR head is fix/lr-c24b-history-pagination-visible-yield, base main)
clagentic gate-note — authorized
Authorize rationale: lr-c24b history-pagination visible-yield fix. PEACHES clean (0 findings, pullrequestreview-4627729667). BOBBIE clean (0 findings, pullrequestreview-4627740287). Task lr-c24b has no comment thread entries blocking merge. |
What changed
Fixes history-pagination paging by raw event count instead of visible-message yield (lr-c24b). Long agentic turns are dominated by invisible-yield events (TodoWrite/TaskCreate/TaskUpdate/TaskList/TaskGet, hidden plan tools EnterPlanMode/ExitPlanMode/ask_user_questions, state events like message_uuid/session_id/status/compacting, thinking deltas) - a HISTORY_PAGE_SIZE=100 raw-event page could render zero visible bubbles, so 'Load earlier messages' appeared to do nothing and a lone sentinel button could show with nothing above it.
Three bounded edits, per the task plan:
Server (lib/sessions.js, lib/project-sessions.js): extend the page window backward turn-boundary by turn-boundary until the slice contains >=1 visibly-rendering event or from reaches 0. Bounded at MAX_VISIBILITY_EXTENSIONS = 5 additional steps, each step itself capped at the existing HISTORY_PAGE_SIZE per findTurnBoundary - no unbounded backward scan (the codex engrams from 2026-05-27/29 flagged this exact failure class). Applied to both load_more_history (scroll-up pagination) and the initial-replay findLastTurnStart window.
Client - prependOlderHistory (lib/public/modules/app-header.js): snapshot the DOM sibling immediately before the prepend anchor prior to rendering a batch. If the batch inserted nothing new and more history remains, immediately request the next page instead of settling loadingMore:false - one user click always surfaces >=1 message.
Client - updateHistorySentinel (lib/public/modules/app-header.js): never render a solitary Load-earlier-messages button with no visible content below it - trigger a load instead. Skipped during an in-progress initial replay (replayingHistory), where an empty messages container is expected transiently, not stuck.
Shared predicate: lib/history-visibility.js (CJS) + lib/public/modules/history-visibility.js (ESM) - isVisibleHistoryEvent() classifies a recorded history event as visible vs invisible-yield, mirroring the existing lib/model-context-windows.js / lib/public/modules/model-context-windows.js CJS/ESM parity pattern (lr-336f) since this repo has no bundler to unify the two module systems. A parity test enforces the two copies stay in sync with each other and with the render dispatch in app-messages.js.
Why
Root cause and full analysis are in the lr-c24b task description (andy). This restores the acceptance criteria: (a) one Load-earlier click always surfaces >=1 visible message when older history exists, (b) the session pane never shows a lone sentinel with no message, (c) no unbounded backward scan - per-step cap preserved, (d) npm test passes.
Sequenced to land before lr-7e8c (virtual scrolling), which reworks the same prependOlderHistory/renderer path.
Test status
npm test: 683/683 passing (641 pre-existing + 42 new/changed in this PR: 30 CJS/ESM predicate-parity cases, 6 server-side bounded-window-extension cases, 6 client-side source-text regression cases for the auto-advance and solitary-sentinel invariants - this repo has no jsdom, so DOM-dependent ESM modules are covered via the existing diagnostics.js-style source-text regression convention).
Task: lr-c24b