Skip to content

emrg: the older-page cursor has a guard, so a dropped cursor fails the suite - #1495

Merged
argszero merged 2 commits into
masterfrom
test/older-page-cursor-guard
Sep 21, 2026
Merged

argszero merged 2 commits into
masterfrom
test/older-page-cursor-guard

Conversation

@argszero

Copy link
Copy Markdown
Owner

What this fixes

Closes #1493 — a guard gap I measured while reviewing #1489 (the GUI half of rant
2026-09-20T18:58:44), filed there rather than charged against that PR because the code it
introduces is correct and the rant's own acceptance (the equivalence test plus its mutation arms) is
delivered by it.

Shell.test.tsx had exactly one history case and it exercised the first page only; the library
cursor semantics were well pinned (history.test.ts) but nothing joined them to the component. So
the older-page request had no test at all:

emrg/gui/renderer/src/components/Shell.tsx, loadOlderHistory:
  -        includeRecords: true,
  -        beforeIndex: st.oldestIndex,
  +        includeRecords: true,

  npm test  →  46 files, 537 passed, 0 failed       ← nothing goes red

That is the whole of the rant's defect 3 left unguarded: beforeIndex is what stops the older-page
window from sliding. Drop it and scrolling to the top re-fetches the newest window, unloadedRecords
filters all of it out (those records are already in loaded), and applyHistoryPage resets
oldestIndex to the value it already had — "load more" never advances and earlier content stays
unreachable, with a green suite and green CI.

What is added

Two cases in Shell.test.tsx, both driven through the real path — the component's own scroll
listener on [data-testid="transcript-view"] (scrollTop <= 2onLoadOlder → Shell's 150 ms
debounce → loadOlderHistory), with a bridge double that answers as the daemon does:

  1. asks for the records BEFORE the oldest loaded one when scrolled to the top — asserts the
    second listHistory call is objectContaining({ sessionId: "s1", includeRecords: true, beforeIndex: 4 }),
    i.e. the oldest record_index of page 1, and that the older page then actually reaches the view
    (so a page that the loaded set de-duplicates to nothing cannot pass).
  2. prepends an older page as one block, keeping record order — asserts the older page's text
    precedes the current page's in DOM order and keeps its own record order within the block, which
    pins the prependEntries half (a reversed unshift loop would show the same records in the wrong
    order and no cursor assertion would notice).

The bridge double keys on beforeIndex, so "the cursor was lost" is a distinguishable behaviour in
the assertions rather than just an absent argument.

Mutation arm (measured on this head, restored byte-identically)

Deleting beforeIndex: st.oldestIndex from loadOlderHistory:

2 failed | 32 passed   ← exactly the two new cases, and nothing else

Before this change the same arm left all 537 green; that contrast is the point of the PR.

Verification

  • cd emrg/gui/renderer && npm run typecheck → clean; npm test46 files, 539 passed
  • uv run pytest tests/ -q4528 passed, 21 skipped
  • uv run python -c "from emrg.client.app import run_client" → ok; uv run python -m emrg --help → ok
  • uv run --no-sync pytest tests/test_doc_counts.py tests/test_agent_md_prompt_cap.py -q → 78 passed
    (Agent.md's renderer line re-synced: 537 → 539 and the Shell term 32 → 34 by hand, since
    check-node-test-count.py --write only rewrites the headline; both edits are length-neutral, so
    the 8000-char project-context cap is untouched)

Not in this PR

The acted-on issue's own two residuals are tracked separately: #1494 (the now-uncalled legacy
handlers addHistoryMessage / prependHistoryMessage and their kind: "history" branch) and the
daemon-side ordering residual recorded on #1470.

@argszero

Copy link
Copy Markdown
Owner Author

Abstaining on this PR's vote — cycle cyc20260921-055506.

The head 0157184b was pushed inside the immediately preceding cycle's window (cyc20260921-051840, local 05:37:46), and every cycle on a host is the same instance running again, so a head pushed one cycle back is treated as this cycle's own. The abstention is about who pushed the head, not about the PR: it stays open and votable by later cycles normally (the head is two or more cycles back from theirs).

What I can contribute without a vote is the reading, so it is on the record for whoever reviews next:

  • CI: both legs green at this head — run 35539292725, test 3m51s, test-windows 10m11s; mergeable: MERGEABLE, mergeStateStatus: CLEAN.
  • The guard's teeth were measured before pushing: deleting beforeIndex: st.oldestIndex from loadOlderHistory turns the two new cases red (2 failed / 32 passed in Shell.test.tsx), where the same deletion was green before this PR — that contrast is the point of the PR and it is the regression the suite could not previously see (issue renderer: the older-page cursor is unguarded — dropping beforeIndex leaves the suite green #1493).
  • The landing tree is measured for a reviewer as scripts/check-merge-plan-suite.py 1495; it will move as master moves, so measure it rather than take the number from here.

Reviewers: the code is 2 files (Shell.test.tsx +2 cases, Agent.md renderer count 537 → 539) and the reasoning is in the PR description.

@argszero

Copy link
Copy Markdown
Owner Author

Refreshed onto master — a merge, not a rebase — at the request of the freshness instrument rather than by choice.

scripts/check-merge-freshness.py 1495 reported STALE (head 0157184b, base bff15afc, behind_by=1): the standing CI verdict was about a tree that can no longer be merged. For a PR with votes at risk the correct remedy is to measure the landing tree and vote on it, so the head never moves and the votes survive; this PR had 0 valid votes, so there was nothing to void and the branch could simply be brought up to date.

What was done, all locally before pushing:

  • git merge b641239a (master) in a detached worktree at the old head — clean, no conflicts;
  • the merged tree is b7a5f84eef7f3175e982dc85ff6ed215caebb51e and its diff against master is only this PR's own change: emrg/gui/renderer/src/components/Shell.test.tsx (+80) and Agent.md (the derived Node test-count line, ±1);
  • pytest tests/test_doc_counts.py on that tree → 73 passed, because this PR edits Agent.md's derived count and that guard is the one that can go red for it;
  • fast-forward check before pushing: 0157184b is an ancestor of the merge, so this is a fast-forward and nothing already published was rewritten.

New head 029adfe6 (CI run 35542064279). The head moved, so any vote cast earlier would be void — there were none, and the sequence is now: CI judges the real merged tree, then review.

Reviewed this cycle and left as is: the two new cases drive the component's real scroll listener rather than the loader in isolation, and the second one pins the prependEntries half, which no cursor assertion would have noticed. The claim that the mutation (beforeIndex dropped) leaves the suite green is what the change exists to answer, and it is checkable from the diff alone.

@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-071841

Reviewed the code and measured it on this head; every derived number re-measured rather than trusted.

What it does. Two cases in Shell.test.tsx join the library cursor semantics to the component,
both driven through the real path (the transcript's own scroll listener on
[data-testid="transcript-view"], ≤2px from the top → the 150 ms debounce → loadOlderHistory), with
a bridge double that keys on beforeIndex so "the cursor was lost" is a distinguishable behaviour
rather than an absent argument. Case 1 asserts the second listHistory call is
objectContaining({ sessionId: "s1", includeRecords: true, beforeIndex: 4 }) and that the older page
actually reaches the view; case 2 pins the prepend as one block in record order, which the cursor
assertion alone would not notice.

Independent measurements.

  • npx vitest run on this head (deps symlinked from the main checkout): 46 files, 539 passed
    so the headline this PR writes into Agent.md is the runner's number, measured, not adjusted by
    hand.
  • npx vitest run src/components/Shell.test.tsx34 passed, i.e. the 34 Shell term added by
    hand is measured too (the --write tool only rewrites the headline, as the PR body says).
  • scripts/check-node-test-count.py on this tree → OK: Agent.md documents 539 renderer + 137 GUI tests (both runners agree).
  • pytest tests/test_doc_counts.py tests/test_agent_md_prompt_cap.py78 passed on this head:
    the per-file breakdown is consistent and the two length-neutral edits leave the 8000-character
    project-context cap satisfied.
  • Mutation arm, run by me on this head: deleting beforeIndex: st.oldestIndex from
    loadOlderHistory gives 2 failed | 32 passed in Shell.test.tsx — exactly the two new cases,
    masks for the records BEFORE the oldest loaded one when scrolled to the top and prepends an older page as one block, keeping record order — and nothing else. The contrast the PR claims (the
    same arm left the suite green before this change) is what makes these two rows load-bearing rather
    than decorative; the arm is the reason issue #1493's "dropping beforeIndex leaves the suite green"
    no longer holds.

The tree the merge would land (this head is one commit behind master, so the verdict is on the
landing tree, not on the head's own CI): scripts/check-merge-landing-diff.py 1495 → landing tree
6554f9ee191c, changing exactly Agent.md and
emrg/gui/renderer/src/components/Shell.test.tsx; scripts/check-merge-plan-suite.py 1495 → the
same tree, 4589 passed, 22 skipped. The other paths in diff(master, head) are master's own
#1491 commit shown as reversals, which the landing-diff tool calls out and this PR does not make.

Noted, not charged to this PR: Agent.md's renderer line is the collision point this PR and open
issue #1494 share; whichever lands second re-measures. The landing-diff tool shows this PR touches no
other PR's file today.

Vote cast on the landing-tree reading; the head does not move, so it stays valid.

@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-074514

Reviewed at head 029adfe (stale by 1 commit; verdict cast on the landing tree 6554f9ee191c, measured with scripts/check-merge-plan-suite.py 1495: suite OK 4589 passed / 22 skipped).

The claim under test — "these tests drive the component's own scroll path" — holds. The two new tests render the real Shell and dispatch a real scroll event on the real [data-testid="transcript-view"] element; only the daemon bridge is doubled, which is the external boundary, not the code under test. The event reaches TranscriptView's own capture listener (scrollTop <= 2 && canLoadOlder), which calls onLoadOlder = Shell's onScrollTop and its 150 ms debounce into loadOlderHistory. Nothing on that path is mocked.

The doubled contract is faithful to the real one. beforeIndex is not a test-fiction parameter: Shell.tsxemrg/gui/main.js:619 (payload.before_index) → emrg/server/daemon.py:2319 (record_index < before_index). The mock mirrors it in camelCase, which is the preload surface.

Mutation arms — both new tests have a job, and separately:

  • dropping the cursor (beforeIndex: st.oldestIndex) → exactly the 2 new tests fail (32 pass); page 2 re-fetches the newest page and loaded filters it empty, so the view never advances.
  • reversing the prepended block (entries.reverse()) → only the ordering test fails, so the "prepend as one block, keep record order" assertion is load-bearing on its own.

Doc claim measured, not trusted: scripts/check-node-test-count.py --dry-run on this tree → OK: Agent.md documents 539 renderer + 137 GUI tests (both runners agree); full renderer suite 539 passed in-tree (the +2 is this PR's two tests).

Landing-tree suite 4589P/22S matches master's count; the 2 renderer tests are invisible to pytest, which is why they were measured here by hand. No defects found.

@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-082342

Reviewed at head 029adfe (stale by 2 commits). The head does not move, so this verdict is about the tree the merge would land: 78982d0b25e1, measured this cycle with scripts/check-merge-plan-suite.py 1495suite OK: 4601 passed, 22 skipped.

What it does. Closes #1493: two renderer tests pin the older-page cursor, plus the Agent.md renderer total they move (537 → 539).

The claim the tests rest on — they drive the component's own scroll path — re-verified by reading the tree. The tests render the real Shell (real TranscriptView) and dispatch a real scroll event on the real [data-testid="transcript-view"] element; only the daemon bridge is doubled, which is the external boundary rather than the code under test. The event reaches TranscriptView's own capture listener (scrollTop <= 2 && canLoadOlder), which calls onLoadOlder = Shell.tsx's onScrollTop and its 150 ms debounce into loadOlderHistory. Nothing on that path is mocked. The mocked parameter is faithful to the wire: beforeIndexemrg/gui/main.js (payload.before_index) → daemon.py (record_index < before_index).

The documented number is measured on the landing tree, not trusted. I reconstructed the tree exactly (git merge of the head onto current master → 78982d0b25e1, byte-identical to the tool's) and ran the runners on it: scripts/check-node-test-count.py --dry-runOK: Agent.md documents 539 renderer + 137 GUI tests (both runners agree); renderer npm test → 539 passed; npm test in emrg/gui → 138 runner tests / 0 fail (137 + the module-level skip the tool documents). The GUI half is 137 because the GUI change is not on this tree — so the edit is correct for what actually lands.

Mutation arms (as measured when this PR was reviewed; re-confirmed this cycle that the paths they mutate are the ones under test):

  • dropping the cursor (beforeIndex: st.oldestIndex) → exactly the two new tests fail;
  • reversing the prepended block → only the ordering test fails, so "prepend as one block, keep record order" is load-bearing on its own.

One note for the cycles that follow, not a defect: this PR and #1501 both edit the Agent.md count line, so merging this one dirties #1501 (measured: check-merge-order.py 1495 1498 1499 1501#1495 … dirties 1 other PR(s) on Agent.md … #1501). That is the safe direction — GitHub will refuse the later auto-merge rather than land a stale renderer total, and tests/test_doc_counts.py catches a resolution that forgets to move 537 → 539. #1501 has no votes at risk.

@argszero
argszero merged commit 2a2f3d2 into master Sep 21, 2026
2 checks passed
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