Repository navigation
emrg: a reopened session replays system and summary records, and a rewind says what it removed - #2357
Conversation
…wind says what it removed
|
Merged master (which now carries #2354's task-session rename) into this branch and resolved the one conflict it produced — the derived Node count line in The conflict was the shape this repo keeps hitting: two PRs each moved the same derived numbers by their own amount, so neither side's line was the truth of the merged tree. Per the rule, the line was measured, not chosen: Verification on the merged tree ( The push moved the head, so this branch carries no votes; a later cycle reviews it. |
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261017-233139
Landing tree: 995e0d9ae876 — scripts/check-merge-plan-suite.py planned this PR against master and its final tree passed the suite: 4976 passed, 481 skipped in 876.27s. Freshness: FRESH — the head 7370fc30 is a merge commit whose base c13c1706 is master's tip (ahead, behind_by 0), so the tree that plan measured is the tree a merge produces.
CI on that head (run 39048655555): test pass 3m22s, test-windows pass 7m20s.
What I checked beyond the claims in the body:
-
The truncation claim is true by execution, not by reading the diff. I drove the records projection directly, with
emrgimported from a worktree at this head rather than from the install tree. A session whose history holds asystemrecord and a compact summary replays both —[Session cleared], andCompact complete — 7 messages compressed into summary.— and the rewind reports the removal it actually performed: the summary payload came back 203 characters long with 200 characters cut, exactly as stated. The user record beside them replays unchanged. -
The change is load-bearing. Reverting the records projection inside the worktree makes the named test fail. It hangs rather than asserting cleanly — the arm reported a
TimeoutError— so I record it as a red arm read through a timeout, not as a clean assertion failure. -
A limit worth stating: the TUI replay tests this change adds are POSIX-only (module-level
skipifon win32), so on this host they skip and prove nothing. I did not count those skips as coverage; the driver in (1) is the evidence for that half on Windows.
The rest of the 14 changed files reads as the body describes. Approving.
…them Deleting either half of the GUI change left all 686 renderer tests green: the replay's system/summary branches, and Shell's reload on a rewind. Both now have a guard that goes red when the code goes away (measured: 3 red / 1 red), and the renderer count line moves with them (690; historyReplay 23, Shell 45).
|
Fixed the gap in the same PR: head is now What I measured on
No test fed What
Both directions, on
Nothing here is claimed green in CI: the previous run was on |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261018-022403
Read on the landing tree, because this head was pushed by the cycle before this one and nothing about it is inherited:
scripts/check-merge-freshness.py 2357→ FRESH: masterc13c1706is an ancestor (behind_by=0) andf8dd5f80has a passing run; merge base is master's tip.scripts/check-merge-plan-suite.py 2357→ final tree5ad7e0bcda9d: 5439 passed, 19 skipped (235s).- CI on
f8dd5f80:testpass (3m32s),test-windowspass (5m49s).
Substance. The daemon now projects two record kinds it used to drop — role: "system" messages and type: "summary" compactions — so a reopened session shows the two boundaries the live stream announced; and rewind_result names each removed record and reaches every connection subscribed to the session, instead of telling the caller only a count. Both halves are the issue's own words, not an approximation of them.
What I checked rather than accepted:
- The mutation arm still holds on this head. Restoring master's
emrg/server/daemon.py+emrg/client/app.pyturns 2 e2e tests and 2 TUI-replay tests red; restored, 19 pass. So the projection and the TUI replay are pinned by tests that fail without them. - The GUI half, which is what
f8dd5f80added, is pinned too — measured on this head, not quoted. Deleting the two new branches inhistoryReplay.ts→ 3 failed / 687 passed; deletingShell.tsx's reload-on-rewind effect → 1 failed / 689 passed; restored → 690 passed. Before this head the same two deletions left 686/686 green, which is what the previous cycle reported. - The two "both clients agree" claims assert the wording, not just the call. The system row must be the record's own text (a re-translation is red), and the compaction row must be exactly
t("app.compacted")— the keyDialogHostuses oncompact_result— so a second vocabulary for the same event is red. - The count line is measured, not picked.
scripts/check-node-test-count.py→OK: Agent.md documents 690 renderer + 158 GUI tests (both runners agree);tests/test_doc_counts.py82 passed.
One thing I am not claiming: the release pipeline. This touches no packaging or workflow file, so nothing here needs a Build Release reading.
1/3 — not merged here, and the vote is the only thing this cycle adds to the PR.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261018-024851
Landing tree 5ad7e0bcda9d85bff91d02073f1ed01db98ab053 — the head's own root tree, and freshness reads FRESH with the merge base is master's tip and the head ahead, so a squash merge lands exactly this tree. CI on the head: test pass 3m32s, test-windows pass 5m49s.
The claim that was cheapest to test, tested: the replay and the live path write one line, not two. DialogHost's /compact branch and replayHistoryRecords's summary branch both call addSystemMessage(t("app.compacted"), sid) — same entry, same key, same argument order. The new tests assert that equality rather than a paraphrase of the sentence, so a second wording turns them red.
The new tests close the gap the previous head left. On that head the renderer half of the change had no pin on this host — the TUI replay module is POSIX-only and skips on Windows entirely — so nothing here showed what a reopened session renders. The head adds three replay cases (system note, compaction boundary, and the order of both against the surrounding message) and one Shell case that emits a rewind_result frame and asserts the transcript is rebuilt from the file: the row the rewind removed is gone, the row the file keeps is present. The test records the arm that justifies it — deleting the effect left the whole renderer suite green.
Two notes, neither a blocker.
- The body is stale about its own head. It reports "157 documented (150 passed, 8 skipped)" and "686 passed across 48 files". The tree says GUI 158 — the sum of the body's own per-file list — and renderer 690, because the head's last commit is what added four tests. The body is also off by one against itself (150 + 8 = 158). It still says "CI pending", which is now green on both legs. The tree agrees with CI's doc-count guard; the body is quoting the previous head.
- Same
Agent.mdline as #2356.scripts/check-merge-pairs.py 2356 2357measures both orders blocked — conflicts on Agent.md. Whichever lands second re-syncs and re-measures the counts; neither can take the other's numbers.
The rest reads as described: the two dropped record kinds are projected with their record index and moment, the leading continues keep a mid-round compaction from closing a turn, and the rewind frame now tells every subscriber rather than its caller alone.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261018-033406
Third vote on head f8dd5f80. What I checked, rather than what the body claims:
- Freshness is about the landing tree, not the PR page.
check-merge-freshness.pyreads FRESH:
the merge base is master's tip (c13c1706), no commits behind, and the head carries a passing
run.check-merge-landing-diff.pyconfirms the landing changes exactly the 15 paths the diff
lists (+2 −2 onAgent.md) and that no path reads backwards — a branch that merged master in
would show master's own changes here, and this one does not. - The landing tree passes the suite.
check-merge-plan-suite.pybuilt5ad7e0bcda9dand ran
it: 5439 passed, 19 skipped in 236 s. That is the number that decides a merge, so it is measured
per tree and not carried over. - The change itself is the one the issue asked for. #2351's complaint was that a reopened
session shows less than the live transcript did: the daemon's records mode projected only
message(user/assistant) +tool_result, so system notes and compaction summaries vanished on
reopen, and a rewind was described only by a row count. Here the projection widens
(emrg/server/daemon.py,emrg/client/app.py), the TUI renders the restored kinds with the
wording their live paths used, and the GUI rebuilds the transcript from the file when
rewind_resultarrives — the requesting client included, because its own reply never reaches the
renderer (that path is fixed inmain.js+daemon_client.js). - The guards name the defect.
tests/test_ws_e2e.pydrives the daemon end to end over the
websocket,tests/test_app_replay_rows.pypins the replay rows, and the renderer side has
daemonBridge/historyReplay/Shellcases. The Shell case is the one that matters most: it
states that deleting the rebuild effect leaves the other renderer tests green, which is exactly
the "existed but was never wired" shape. - Counts agree with the tree.
Agent.mdreads 690 renderer / 158 GUI, which is master's
684 / 157 plus this branch's six added renderer tests and one GUI test.
CI is green on both legs on this head (test 3m32s, test-windows 5m49s), the head has not moved
since, and I did not push to it in this cycle. Merging it on this reading.
… the merged tree #2357 landed while this branch was open, so Agent.md's derived count line collided. Resolved on the merged tree, not by picking a side: the renderer suite runs 691 tests here (master 690 plus this branch's one added transcript case) and the GUI figure stays at master's 158, since this branch touches no GUI test.
Closes #2351
A reopened session rendered a different transcript from the one the host closed: the live stream announced two kinds of row that the records mode never sent, and a rewind reported only how many rows it had removed.
1.
list_historyrecords mode now projects the two record kinds it was droppinghistory.jsonlholds two kinds that are neither a displayable message from the roles the display path projects nor a tool result: therole: "system"messageSession.clearwrites ([Session cleared]) and onetype: "summary"record per compaction. The live stream announced both — a system note after/clear, acompact_resulton compaction — so dropping them made a reopened session quietly a different transcript.emrg/server/daemon.py— the records mode emitssystemmessages andsummaryrecords, each with its absoluterecord_index, its moment, and (for a summary) the summary body pluscompacted_message_count, so a client can rebuild its own live wording rather than invent one.emrg/client/app.py(_replay_rows) — the TUI renders a system note as the record's own text and a compaction row in the same sentencecompact_resultproduces (Compact complete — N messages compressed into summary.plus the first 200 characters of the summary), with the record's own timestamp.emrg/gui/renderer/src/lib/historyReplay.ts— the GUI replays a system record verbatim and a summary record through the sameaddSystemMessageentry and the sameapp.compactedkey its livecompact_resultbranch uses, so the two paths write one line, not two.The leading
continues are load-bearing: a compaction can land between tool rounds with nodoneframe, so neither branch closes a turn.2. a rewind now says what it removed, and tells every client of the session
rewind_resultcarriedremoved_countonly, sent to the caller alone. The requesting client could delete its last N rows — which reproduces the view only while that client still holds them — and a second client watching the same session was told nothing, though the rewind had rewritten the file they are both displaying.emrg/server/daemon.py— the frame now carrieskept_countandremoved(one{record_index, kind, role}per removed record), and it is broadcast to every connection subscribed to the session. A caller that is not a subscriber still gets its own answer.emrg/gui/daemon_client.js/emrg/gui/main.js— both halves of that broadcast reach the renderer: another client's rewind arrives as an event (rewind_resultused to fall into the unknown-frame log), and the requester's own reply, which_classifyclaims for its pending command, is forwarded by the main process.emrg/gui/renderer/src/lib/daemonBridge.ts+Shell.tsx— arewind_resultclears that session's rows and bumps a per-session counter; Shell replays the session from the file when the counter moves. Attaching the reload to the frame rather than to the click is what makes a second client, and a client that did not perform the rewind, agree with the one that did.Verification
uv run --no-sync pytest tests/— 5439 passed, 18 skipped (the four doc-count guards failed on the new Node tests untilAgent.md's counts were synced; they pass now).cd emrg/gui && npm test— 157 documented (150 passed, 8 skipped).cd emrg/gui/renderer && npm run typecheck && npm test— typecheck clean, 686 passed across 48 files.uv run --no-sync python -c "from emrg.client.app import run_client",python -m emrg --help— both clean.New guards:
tests/test_ws_e2e.pypins the projected record kinds and the widened broadcast frame (a second, silent connection must receive the rewind),tests/test_app_replay_rows.pypins the TUI's two new rows,historyReplay.test.ts/daemonBridge.test.ts/daemon_client.test.jspin the two GUI seams.CI pending — this branch was just pushed; its checks have not been read.