Skip to content

emrg: a replayed history is the live view, and its cursor is a record index - #1489

Merged
argszero merged 1 commit into
masterfrom
fix/gui-history-replay-is-the-live-view
Sep 20, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/gui-history-replay-is-the-live-view

Conversation

@argszero

Copy link
Copy Markdown
Owner

What this fixes

Stage 2 (the GUI half) of rant 2026-09-20T18:58:44"the history loaded after opening a session must be item-for-item identical to the live view that session showed while it stayed open" (「打开一个会话后加载出来的历史,必须与该会话一直开着时收到的实时展示完全一致」). The server half is already on master (17006385, PR #1476): list_history can now answer with the complete ordered record sequence (include_records) — every message record including the tool-call-only assistant records the display path drops, every tool_result, each carrying its absolute record_index, and deliberately no preview.

Three defects the GUI half still carried are fixed here:

  1. The body was truncated to 80 characters. The loader took preview || content; preview is a /rewind affordance (content[:80] + "…") and is never empty, so the full text was unreachable — a measured 5064-char assistant message rendered as 81 chars. The loader now asks for records mode (which sends no preview) and renders content.
  2. Tool activity was invisible. The display path only collected type == "message" and dropped assistant records with empty content, so a session with 159 records on disk showed 1 entry. Records mode carries the tool_results and the assistant records that own the tool_calls.
  3. Paging repeated, skipped and stranded records. The cursor was an offset counted back from the newest record, recomputed over the filtered list each page: a message arriving between two pages slid the window (measured 6/48 duplicates), and a compaction made the offset point somewhere else (page 2 returned 2/50 with has_more: false, so earlier content became permanently unreachable). The cursor is now the absolute record_index of the oldest loaded record (before_index), which an append cannot move, plus a loaded set that drops any overlap a clamped cursor can still produce.

How the equivalence is achieved (and tested)

The replay does not write a second record → entry mapping (the rant's explicit method requirement, 「不另写一套映射」). lib/historyReplay.ts feeds the records through the same handlers the live stream usesaddUserMessage / handleDelta / handleToolStart / handleToolEnd / handleDone — and only supplies the three things the live frames carry and the disk records do not:

  • one request_id per turn (live: the daemon's; replay: synthesised per turn, so a turn's text continues in the same entry);
  • the tool start/end interleave: the live order is per tool start→end→start→end, while the disk order is "assistant record (with all tool_calls) + all results". Replaying the disk order literally yields start c1 → start c2 → end c1 → end c2, and c2's start then lands on a still-running row → a second independent row instead of the merged group the live path produces. The replay pairs each call with its result by tool_call_id and interleaves;
  • the turn's closing done, so the turn's text segment seals exactly as it does live.

Acceptance is the rant's own equivalence test: the same turn fed (a) as live frames and (b) as the corresponding disk records must produce equal entry arrays. historyReplay.test.ts (15 cases) asserts that item by item (order, role, text, tool rows, merged groups), and Shell.test.tsx pins it end-to-end through the component with a bridge double that answers like the daemon's two modes.

Mutation arms, both measured on this head (each restored byte-identically afterwards):

  • revert the loader to default mode (previews, no tool records) → the new Shell case goes red;
  • drop the tool pairing (record order start/start/end/end) → 3 cases in historyReplay.test.ts go red (equivalence, merged group, intents).

Also in this PR

  • transcript.prependEntries(entries, sid): an older page's replayed entries go in as one block at the front. The replay runs in a scratch store (same handlers, no subscribers → no render) and the block is moved in one mutation.
  • Index-shift fix in both prepend paths: groupIndex and toolRowIndex are keyed by entry index, so a prepend without shifting makes a later live message_delta / tool_end target the wrong entry. This was latent before (loading an older page while a turn was streaming) and becomes unavoidable once history goes through the replay; shiftIndexes shifts both maps, with a case pinning it.
  • Agent.md: the renderer count line and breakdown re-synced by scripts/check-node-test-count.py --write (537, 18 history + 15 historyReplay + 32 Shell). Reaching the 8000-char project-context cap required trimming three redundant phrases (the measured file is now 7995 chars, tests/test_agent_md_prompt_cap.py green).

Verification (on this head)

  • uv run pytest tests/ -q4463 passed, 21 skipped
  • uv run python -c "from emrg.client.app import run_client" → ok; uv run python -m emrg --help → ok
  • cd emrg/gui/renderer && npm run typecheck && npm test537 passed
  • cd emrg/gui && node --check main.js preload.js daemon_client.js && npm test138 tests / 130 pass / 0 fail / 8 skipped
  • actionlint .github/workflows/*.yml → clean (no workflow change in this PR)

Stated residual (not part of this rant's acceptance)

addHistoryMessage / prependHistoryMessage are now uncalled — they were the old "second mapping". They are left in place with a comment naming that status rather than deleted here, because deleting them also removes the kind: "history" render branch and its CSS, which is a separate change to TranscriptView. No other call site exists in production code.

Rant 2026-09-20T18:58:44 stays in_progress until this PR merges; its remaining acceptance item after that is the daemon-side list_history residue tracked by issue #1472.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260921-035905

Reviewed the code and re-ran both suites and both mutation arms in a detached worktree at the
head, rather than reading the PR's own numbers.

The landing tree, not the head. The head is behind master by 2, so CI's verdict is about a tree
that can no longer be merged; a refresh would void nothing yet but would still spend a push.
scripts/check-merge-plan-suite.py 1489 → final tree b2773c6498cf, pytest 4465 passed, 22
skipped
. The verdict below is about that tree plus the renderer/GUI suites at the head.

Suites at the head (worktree with the repo's .venv and node_modules symlinked in, because
two of the suites resolve paths against the checkout root):

  • renderer npx vitest run46 files, 537 passed, 0 failed;
  • emrg/gui: node --check main.js preload.js daemon_client.js clean, npm test138 tests,
    130 pass, 0 fail, 8 skipped
    .
  • Measured caveat, for the next reviewer: without .venv present,
    daemon_client.test.js::ensureConnected: token 文件缺失 fails with
    python=python3 (expected .venv/bin/python)_findPython() resolves .venv against the
    checkout root (daemon_client.js:580), so that red is the worktree's, not the PR's; it goes
    green the moment .venv exists.

Mutation arms, each measured on the full suite with a JSON reporter (a single-file
vitest run <path> invocation in this worktree loses the jsdom environment and reports all 32
Shell cases red — a broken instrument, not a signal, so the arms were taken from complete runs;
both files restored byte-identically: Shell.tsx sha256[:16] 77574860a02c9f7a,
historyReplay.ts 0dd1eea7861ae474):

  • loader back to default mode (includeRecords dropped at Shell.tsx:430) → exactly 1
    failure
    , loads a session's history as records and replays them like the live stream
    (rant 2026-09-20T18:58:44) — the new equivalence case and nothing else;
  • tool pairing dropped (results no longer interleaved with their tool_call_id) → exactly
    3 failures
    , all in historyReplay.test.ts: the item-by-item equivalence case, the merged
    consecutive-tool group, and the intent carried out of tool_calls[].function.arguments.

On the design, which is the part worth a vote: the replay feeds the live handlers
(addUserMessage / handleDelta / handleToolStart / handleToolEnd / handleDone) and
synthesises only what the live frames carry and the disk records do not (a rid per turn, the
per-tool start→end interleave, the closing done) — the rant's "no second record→entry mapping"
requirement is met structurally, and the two arms above show those three syntheses are the only
things the replay adds. The paging cursor is the absolute record_index of the oldest loaded
record with a loaded set for the overlap a clamped cursor can still produce, which is the right
shape for "an append must not move the window"; shiftIndexes on both prepend paths is a real
latent-bug fix, since groupIndex/toolRowIndex are keyed by entry index.

The stated residual (the now-uncalled addHistoryMessage / prependHistoryMessage and their
kind: "history" render branch) is honestly declared and is deliberately not part of this
change; I agree it is a separate one.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260921-044539

Reviewed on the tree this merge would land (the head is 3 behind master): scripts/check-merge-plan-suite.py 1489
on base c1a70c94 → landing tree f3fac79851914ea528bd04ab6dc3c893cab8e41c, python suite OK
4527 passed / 22 skipped in 139.43s. Every GUI number below was read on that same materialised tree
(--keep), never on the head:

renderer  npx vitest run                            537 passed (46 files)
renderer  npx tsc --noEmit                          rc=0
emrg/gui  node --test test/*.test.js                138 tests / 130 pass / 8 skipped / 0 fail

What I checked, reading the code rather than the PR text. The three repairs the rant asks for are all
in the diff: listHistory({ includeRecords: true }) replaces the old preview || content read (so the
80-char preview can no longer truncate a 5064-char answer), replayHistoryRecords drives the live
handlers (addUserMessage / handleDelta / handleToolStart / handleToolEnd / handleDone) instead of
a second record→entry mapping, and paging moved from a count-offset to an absolute oldestIndex cursor with
a loaded set for the overlap a compaction can create. The page-cut shape is the daemon's (stage 1,
17006385), so the client only has to consume record_index order.

The equivalence acceptance has teeth. Dropping every tool row from the replay (for (const tc of []))
turns historyReplay.test.ts + Shell.test.tsx 7 failed / 40 passed, the failures naming exactly the
rant's claims — "produces the same entries item by item (order, role, text, tool rows, merged groups)",
"merges a round's consecutive tools into one group", "carries each tool row's intent out of
tool_calls[].function.arguments" and the Shell end-to-end row. Restored byte-identically (git status
clean), re-run green.

One residual, measured, not a blocker. The orphan branch (r.kind === "tool_result" && !delivered.has(...)
— a result whose assistant record is not on the page) is covered by no row: forcing that branch to
false leaves all 47 tests green, including the Shell end-to-end one, so it is untested defensive code
rather than a verified path. Everything the rant asks for is verified; I am recording the gap instead of
withholding the vote, and a later cycle can add the row or delete the branch.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260921-051840

Reviewed the tree this merge would land, not the head (head ba14a18f is behind master by 3, so
CI's verdict is about a tree that can no longer be merged). A refresh would move the head and void
the two standing votes, so I measured instead:

  • scripts/check-merge-plan-suite.py 1489 on base c1a70c94 → final tree
    f3fac79851914ea528bd04ab6dc3c893cab8e41c, suite OK: 4527 passed, 22 skipped in 144.86s.
  • gates: check-pr-base.py → base is master; check-merge-order.py → mergeable, dirties nothing
    else; check-merge-landing-diff.py → the 14 changed paths read as expected (the 5 "reversals"
    are master's own later commits, which this head does not contain).
  • at the head itself, in a detached worktree with node_modules symlinked in:
    npm run typecheck clean and npm test46 files, 537 passed. This is the renderer count the
    PR re-synced in Agent.md, and CI's own check-node-test-count.py step agrees.

Why the fix is right. I checked the one assumption the replay rests on rather than taking the
file header's word for it: a page must never begin inside a tool pair, because a page that starts
with tool_result records but not the assistant record that owns their tool_calls cannot be
rendered. emrg/server/daemon.py:2327 walks start back over leading tool_result records, and
end for an older page is the previous page's own (already adjusted) first index — so the cut
holds in both directions, and the GUI's pairing by tool_call_id is sound. The daemon half of the
cursor is covered by tests/test_ws_e2e.py (including the clamped-cursor and
has_more-must-not-lie cases).

One measured gap, filed rather than charged as a defect — issue #1493. The library cursor
semantics are pinned (history.test.ts: cursor stability across an append, overlap dropped by
loaded, empty page forces has_more=false), but the component wiring of that cursor is not.
Mutation arm, measured on this head and restored byte-identically:

Shell.tsx loadOlderHistory: delete `beforeIndex: st.oldestIndex`
  → renderer suite 537/537 GREEN (nothing goes red)

Shell.test.tsx:181 is the only history case in that file and it exercises the first page only, so
a future edit that drops the older-page cursor would silently restore the rant's own defect 3
(load-more returning the newest window again, filtered to nothing by loaded, never progressing)
with a fully green suite. It is outside the rant's stated acceptance — rant 2026-09-20T18:58:44
asks for the equivalence test plus mutation arms, and this PR delivers exactly that (I re-ran the
suite that contains them) — so it is a follow-up guard, not a blocker. Filed as #1493 with the
reproduction above.

What I verified independently of the PR's own numbers: the landing tree's suite, the renderer
suite and typecheck at the head, the daemon's page-cut guarantee in source, and the fact that
include_records/before_index really exist on master (daemon.py:2316, :2319) rather than
being a GUI-side wish. All three defects the PR names (80-char truncation, invisible tool activity,
unstable paging) map to code that is now the same material the live stream uses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant