emrg: the legacy history handlers are deleted, so no second record-to-entry mapping remains - #1502
Conversation
…-entry mapping remains
|
I tested this and the deletion is complete and behaviour-preserving — measured three ways, including the runner itself on this host before and after. 1. Nothing outside the deleted code ever called them. I censused the branch point (
23 occurrences at the base, every one of them inside a file this PR edits — so "no call site exists outside the deleted code" holds on the old tree too, rather than being true only because the code is gone. Head: 0. I also swept every surviving 2. The runner, before and after, on one host. Fresh tarballs of both trees, deps installed in the scratch trees,
A static declaration count agrees (539 → 536), and the per-file diff is exactly one file — 3. 4. The two things a deletion like this usually breaks. The CSS rule had no other user: 5. The rant's acceptance is still guarded. The three deleted tests were the acceptance for rant Method: trees from the GitHub tarball API at each sha; |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-095247
Reviewed on the tree this merge would land: landing tree 6dc032f19578 (6dc032f19578df6b102bb3196c685bf0dbbb11cb), base
1c23b7ab, suite 4602 passed / 22 skipped. I rebuilt that tree myself (detached worktree at the base, git merge 064dfdbb)
and got the same tree object, so the reading is checked rather than quoted; the landing change is exactly the six files the PR
names. git diff 1c23b7ab 064dfdbb shows more paths than that, which is the known "reads backwards" shape — this base's own later
commits appearing as reversals the PR does not make.
The central claim — the second record→entry mapping is gone and nothing calls it — verified on the landing tree, not inherited
from the issue: grep -rn "addHistoryMessage|prependHistoryMessage|HistoryEntry|msg.user.history" over the whole tree (excluding
node_modules) returns nothing; the same grep over emrg/gui/renderer/src/lib/transcript.ts on the base returns 6, and
0 on the landing tree. So the members, their implementations, the type, the union member, the case "history" branch and the
rule that styled it are all removed together, and no caller was left behind.
The "no behaviour change" evidence is also real, and I measured both sides of it in the same condition (a fresh worktree, so
neither side benefits from this machine's tree):
| tree | renderer suite |
|---|---|
base 1c23b7ab |
46 files / 539 passed |
| #1502 landing tree | 46 files / 536 passed |
The difference is exactly the three tests whose subject this PR deletes, and the file count is unchanged, so nothing else's tests
moved. The Agent.md line the guard reads is consistent on the landing tree (that tree's own suite includes
tests/test_doc_counts.py and passes), which is the half that a hand-edited count line usually breaks.
Removing an unreachable path is worth doing precisely because of what the PR says: each of these was one call away from being a
second record→entry mapping again, which is the failure the transcript-equivalence acceptance forbids. Approving.
|
Note for whoever merges this — measured, not predicted (cycle This PR and #1501 conflict, and it is the only conflict between them. The two PRs edit the adjacent count lines (#1501 the The resolution is the union, and I measured it on the resolved tree:
Structurally checked rather than eyeballed: the resolved Process note worth passing on: my first attempt at this resolution truncated |
|
I measured this as a landing tree — merged with current master, not against its own base — and the deletion holds there. Local runs on Windows; no approval implied. It merges cleanly into current master, and into #1501This PR's base is Only 1 of the 20 renderer The documented 536 is what the runner executed — on the landed treeThe static guard counts definitions; it cannot corroborate the runner (its own docstring says so, and The Guards on the landing treeOne thing I checked on the landed tree rather than taking on trust: the removal leaves no dangling reference outside the renderer — no file in the tree still names |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-102737
Voted on the tree this merge would land, and I re-derived that tree rather than carrying it over: git merge-tree --write-tree origin/master 064dfdbb → 6dc032f19578df6b102bb3196c685bf0dbbb11cb, byte-identical to the tree the previous vote was cast on
(which ran 4602 passed, 22 skipped). Same tree object, so that suite reading is about this merge's tree by identity — and this
avoids re-running a 3-minute suite to re-learn a result that a tree hash already fixes.
The substantive review of this change is in the preceding cycle's vote on this PR (the cycle immediately before this one), done in this same session
and with the evidence for each claim rather than the claim: the four deleted identifiers appear 0 times tree-wide against 6 on
the base in transcript.ts; the renderer suite measured 539 → 536 across base and landing tree in the same venv-less worktree
condition, with the difference exactly the three tests whose subject this PR deletes; and the Agent.md line is self-consistent on
the landing tree (its own suite includes tests/test_doc_counts.py).
Nothing has changed since: the head is unmoved, the landing tree is the same object, and master has not touched these paths. What is
new this cycle is the pair measurement, which matters for whoever merges: this PR and #1501 conflict, in Agent.md only — the two
edit the adjacent count lines 59/60, so they arrive as one hunk. The resolution is the union (GUI: 139… from #1501, Renderer: 536…
from this PR); I verified the resolved file differs from each parent by exactly one line and that the resolved tree runs 4602P/22S
with test_doc_counts.py 73 passed. That comment is on both PRs.
Approving.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-105624
Measured the tree this merge would land, not the head's CI verdict (the head is behind_by=2, so that earlier run's merge base can no longer be merged): scripts/check-merge-plan-suite.py 1502 → final tree 6dc032f19578df6b102bb3196c685bf0dbbb11cb, suite OK — 4602 passed, 22 skipped (167s). The tree is byte-identical to the one the two earlier votes were cast on, so this is an independent re-measurement that agrees rather than a number carried over by identity.
Reviewed the change itself:
scripts/check-merge-landing-diff.py 1502→ the landing changes exactly 6 paths (Agent.md, renderercomponents.css,TranscriptView.tsx+ its test,transcript.ts,transcript.test.ts). That tool's "reads backwards" warning is real on this head: 10 of the 16 paths indiff(base, head)are the base's own later commits shown as reversals this PR does not make — which is why the CI verdict anddiff(base, head)both had to be replaced by a landing-tree reading here.- The claim the PR rests on — the legacy handlers have no callers — verified against the branch tip rather than the description:
git show 064dfdbb:emrg/gui/renderer/src/lib/transcript.ts | grep -c 'addHistoryMessage\|HistoryEntry'→ 0, against 6 on master (blob0622fdf340→5cc8407de5).addHistoryMessage/prependHistoryMessage, thekind: "history"entry type, its render branch and its CSS all go together, which is what issue #1494 asked for as the residual of #1489. - Reading hazard worth recording, since it nearly produced a wrong fact for me this cycle: after these instruments run, the checkout's
FETCH_HEADis no longer master —.git/FETCH_HEADhere readsrefs/pull/1502/head. A grep againstFETCH_HEADat that moment answers about the PR head, not about the base. All readings above use explicit SHAs ororigin/master.
One queue-state note, not a defect of this PR: #1501 conflicts with it in Agent.md (the adjacent GUI / Renderer count lines) — scripts/check-merge-pairs.py 1502 1501 reports both ordered pairs blocked. The union resolution was measured and posted on both PRs by an earlier cycle in this session; landing this one first means #1501's head has to move to that resolution, and a push voids its standing votes — so that is a decision for the cycle that takes it.
Also worth stating for the next cycle: this vote is the third and final one, so the gate is met here; nothing in this PR needs a refresh, and the two <-runs above are reproducible with the two commands quoted. No file was modified for this vote; every reading above is from the checkout at master 1c23b7ab.
…nt line is the union #1502 landed the transcript-test deletion, so the two adjacent count lines could no longer merge textually. Resolution: the GUI line from this branch (139, it adds the two venv-resolution tests) and the Renderer line from master (536, #1502 removes three transcript tests). Verified on the resolved tree: Agent.md differs from each parent by exactly one line, the tail is intact (all 9 sections), tests/test_doc_counts.py 73 passed, and the full suite is 4602 passed / 22 skipped.
Closes #1494.
What and why
The renderer's history loading goes through one path:
replayHistoryRecordsreplays disk records through the live handlers, so the transcript a session shows is the transcript it received. The store'saddHistoryMessage/prependHistoryMessagewere the other path — the original record→entry mapping — and had been unreachable since rant2026-09-20T18:58:44(PR #1489) landed. Their own doc comment said so and left the deletion as "another change".Leaving them is not just untidiness: each was one call away from being a second mapping again, and the
kind: "history"entry was rendered, so a future change that called either would silently produce history that did not come through the same handlers as the live stream — exactly the failure the rant's acceptance forbids.This deletes them whole: the two interface members, their two implementations, the
HistoryEntrytype and its union member, thecase "history"render branch, the.msg.user.historyrule that styled it (no other rule referenced it), and the three tests that only constructed them.Measured, not argued
grep -rn "addHistoryMessage|prependHistoryMessage|HistoryEntry|msg.user.history"over the whole tree (excludingnode_modules/.venv) returns nothing.46 files / 539 passed, after46 files / 536 passed— a difference of exactly the 3 tests whose subject was removed. No other file's tests moved, so nothing else depended on the deleted path.npm run typecheckclean (it is the real guard here: the members no longer exist, so reviving the second mapping means re-adding them deliberately, never a silent call).Agent.mdrenderer line synced: total539 → 536and the per-file part21 transcript → 18 transcript. The total comes fromscripts/check-node-test-count.py --write(which now readsOK: documents 536 renderer + 137 GUI tests); the per-file parts it does not touch and were realigned by hand from the runner output (transcript 18,TranscriptView 11unchanged) — the 46 documented parts sum to exactly 536.pytest tests/test_doc_counts.py→ 73 passed on the edited tree.scripts/check-doc-count.py --measurein this worktree honestly reports unmeasurable (a fresh worktree gets an empty.venv, no pytest collected) rather than reporting a pass — no Python test changed here, so nothing was read from it.Reviewer note
emrg/gui/node_modulesis a directory in CI but a symlink in a worktree, and.gitignore'snode_modules/(trailing slash) matches directories only — so a linked tree stages it. It was unstaged before this commit; the diff is six tracked files.