Skip to content

fix(history): place a fresh entry after the prefix it follows, and hold the cursor - #1081

Merged
Juliusolsson05 merged 2 commits into
mainfrom
fix/history-merge-order
Sep 20, 2026
Merged

Juliusolsson05 merged 2 commits into
mainfrom
fix/history-merge-order

Conversation

@Juliusolsson05

Copy link
Copy Markdown
Owner

Refs #910 (items 2 and 3).

Two deferred findings from the OpenCode Terminal round-2 verification. Both live in the same merger, both corrupt what the user reads, and neither errors.

Item 2 — a fresh row was inserted before the prefix it should follow

A pane holds [a,c]. A chunk arrives as [b,c], with a < b < c. The merger pushed fresh b straight out and only then flushed the window through anchor c, producing [b,a,c]. uuid dedup makes that order permanent.

A fresh entry belongs immediately before the next anchor it shares with the window — which means after everything the window holds ahead of that anchor. The loop now flushes that prefix first.

Only when a following anchor exists. With none, the chunk shares nothing further with the window and the prepend is deliberate: a chunk that does not reach back far enough must not be interleaved on a guess, which is what the unanchored rule above already decided.

Item 3 — a no-op tail refresh moved the pagination cursor forward

Re-hydrating a tail the window already holds produces an all-anchor placement: nothing is added, so appendedAfterWindow is false, and the loader read that as "this chunk is now the window" and moved the cursor to the chunk's head.

With a window of [c,d] that had already paged back to [a,b] and then re-read [g,h], the marker moved to g and the next older page landed above everything: [e,f,a,b,c,d,g,h]. Same misorder the append rule exists to prevent, arrived at from the other side.

The cursor names the oldest entry the pane holds. A load that contributed no entry cannot have changed which one that is.

On the tests

placeHistoryEntries is exported for testing. The rule is a pure function of a chunk and a window, and every bug it has had is one array in and one array out; driving it through the loader and a database would test the same rule through three layers that can hide it.

The cursor half is different, and is driven through the real loader over a real OpenCode database — because the pagination marker is produced by the raw-entry mapper, not by a field a stub can set.

The first version of that test used hand-shaped entries and passed vacuously: with no marker derived, the cursor could not move whether the fix was present or not. Its own control test is what exposed that. Both directions are pinned now.

mutation result
fresh pushed before the prefix (the bug) 3 failed
flushes even with no following anchor 1 failed
flushes past the anchor as well 3 failed
the unanchored append is dropped 1 failed
the cursor moves on a no-op refresh 1 failed
the cursor NEVER moves 2 failed
restored 160 passed

One equivalent mutant is stated rather than covered: dropping the already-emitted half of nextAnchorIndex's guard cannot change an answer, because a stale index is below next and the flush loop is a no-op for it. It stays for symmetry with the anchor branch.

npx tsc -b clean; src/renderer/src/workspace/hook/ 265 passed.

Not in this PR

#910's other two findings. Item 1 (the exit flush skipping its own retry window) is in the opencode-terminal-headless submodule and needs its own PR there. Item 4 (the host never reaching the package's database-generation selection) is a low whose issue text explicitly offers "state the limitation honestly" as an acceptable resolution.

🤖 Generated with Claude Code

…ld the cursor

Two deferred findings from the OpenCode Terminal round-2 verification (#910
items 2 and 3). Both live in the same merger, both corrupt what the user
reads, and neither errors.

ITEM 2 — A FRESH ROW WAS INSERTED BEFORE THE PREFIX IT SHOULD FOLLOW.
A pane holds [a,c]. A chunk arrives as [b,c], with a < b < c. The merger
pushed fresh `b` straight out and only then flushed the window through anchor
`c`, producing [b,a,c]. uuid dedup makes that order permanent.

A fresh entry belongs immediately BEFORE the next anchor it shares with the
window — which means AFTER everything the window holds ahead of that anchor.
The loop now flushes that prefix first.

Only when a following anchor exists. With none, the chunk shares nothing
further with the window and the prepend is deliberate: a chunk that does not
reach back far enough must not be interleaved on a guess, which is what the
unanchored rule above already decided.

ITEM 3 — A NO-OP TAIL REFRESH MOVED THE PAGINATION CURSOR FORWARD.
Re-hydrating a tail the window already holds produces an all-anchor placement:
nothing is added, so `appendedAfterWindow` is false, and the loader read that
as "this chunk is now the window" and moved the cursor to the chunk's head.
With a window of [c,d] that had already paged back to [a,b] and then re-read
[g,h], the marker moved to `g` and the next older page landed ABOVE everything
— [e,f,a,b,c,d,g,h]. That is the same misorder the append rule exists to
prevent, arrived at from the other side.

The cursor names the OLDEST entry the pane holds. A load that contributed no
entry cannot have changed which one that is.

ON THE TESTS. `placeHistoryEntries` is exported for testing: the rule is a
pure function of a chunk and a window, and every bug it has had is one array
in and one array out. Driving it through the loader and a database would test
the same rule through three layers that can hide it.

The CURSOR half is different and is driven through the real loader over a REAL
OpenCode database, because the pagination marker is produced by the raw-entry
mapper rather than by a field a stub can set. The first version of that test
used hand-shaped entries and PASSED VACUOUSLY — with no marker derived, the
cursor could not move whether the fix was present or not. Its control test is
what exposed that, and both directions are pinned now.

Mutation-checked, 160 tests:

  fresh pushed before the prefix (the bug)   3 failed
  flushes even with no following anchor      1 failed
  flushes past the anchor as well            3 failed
  the unanchored append is dropped           1 failed
  the cursor moves on a no-op refresh        1 failed
  the cursor NEVER moves                     2 failed
  restored                                 160 passed

One equivalent mutant is stated rather than covered: dropping the
already-emitted half of `nextAnchorIndex`'s guard cannot change an answer,
because a stale index is below `next` and the flush loop is a no-op for it.

Refs #910

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h is bounded by time

Review findings on #1081.

FINDING 1 (high, and a regression this PR introduced): item 2's prefix
flush changed which row ends up at `merged[0]`, and the cursor rule did
not follow. `appendedAfterWindow || !addedFreshEntries` was true before
the flush, when a chunk starting with a fresh row really did put it
first. After it the merged head is the WINDOW's head while
`addedFreshEntries` is still true — so the cursor jumped onto a row the
pane holds in the MIDDLE, and the next older page prepended above a row
that precedes it. Permanently, because uuid dedup fixes the order. The
fix to item 2 had recreated item 3 from the other side.

`placeHistoryEntries` now reports `keptWindowHead` — observed, not
inferred: either the merged result still begins with the window's first
entry or it does not. That is exactly the question the cursor asks, and
it subsumes both old signals (an append keeps the head; an all-anchor
re-read keeps the head). The regression is pinned at the LOADER seam over
a real OpenCode database, with the reviewer's own scenario: a pane
holding [msg_1, msg_4, msg_5] receiving [msg_3, msg_4, msg_5].

FINDING 2 (medium): "a fresh entry belongs after everything the window
holds ahead of that anchor" was a guess presented as a derivation. The
window can hold a row the chunk skips, and that row can fall on either
side of the fresh one — flushing unconditionally fixes one family and
breaks the other:

  window [a,c]   chunk [b, *c]         a < b  → flush a, then b
  window [b,d,e] chunk [*b, c, *e, g]  d > c  → must NOT flush d

The timestamps are already on these entries and are what tells the two
families apart, so the flush now stops at the first retained row strictly
NEWER than the fresh one. When either side is undated the comparison is
false and the row is flushed — the pre-timestamp behaviour, and the
better default by 12,275 to 1,397 on the reviewer's fuzz.

The trimmed-anchor test asserted an order its own input refuted: a trim
drops the OLDEST side of the window, so every trimmed uuid is older than
every retained row, which made the chunk's stated order impossible.
Rewritten with consistent timestamps, and the file now says that every
placement must be authored in durable order.

FINDING 3 (medium): the "equivalent mutant" claim about `index < next`
was FALSE. A stale index would make the caller's flush loop a no-op — but
skipping it lets the scan continue to a later anchor that does flush.
Same input, different output, with the reviewer's example now in the
comment and in a test.

FINDING 4 (low): the two remaining survivors are closed — the
empty-window guard on `keptWindowHead`, and a tie in `isAfter` (equal
timestamps are not strictly newer, so they flush, matching
`isStrictlyNewer` on the unanchored path). The cursor fixture now slices
`offsets` alongside `entries`, which the loader documents as parallel.
`historyOldestOffset`'s `initialOldestMarker !== null` half IS equivalent
today and the comment now says so, with the reason.

FINDING 5 (low): the export's justification was wrong — the loader seam
is not one that hides this rule, as the cursor half of the PR shows. The
comment now gives the real reason and names finding 2 as this export's
cost.

Ten mutations, no survivors: the old cursor pair, `keptWindowHead` forced
either way, the empty-window guard, dropping the timestamp bound,
reverting the flush entirely, the stale-anchor guard, a tie, undated
treated as newer, and flushing past the anchor.

Refs #910

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Adversarial review returned REQUEST CHANGES. Point by point — finding 1 is a regression this PR introduced, and it is the one that matters.

1. The prefix flush took the cursor with it (HIGH) — valid, fixed

Correct, and exactly right about the mechanism: appendedAfterWindow || !addedFreshEntries was a true statement before item 2. A chunk starting with a fresh row really did put that row at merged[0], so "added something → the chunk's head is now the oldest" held. The flush falsified the premise and left the conclusion. The fix to item 2 recreated item 3 from the other side, which is precisely what item 3's own comment claims it exists to stop.

The seam you named is the one I took: placeHistoryEntries now returns keptWindowHead — observed, not inferred:

keptWindowHead: existing.length > 0 && merged[0] === existing[0]

That is the question the cursor actually asks, and it subsumes both old signals: a strictly-newer append keeps the head, an all-anchor re-read keeps the head, the deliberate unanchored prepend does not. addedFreshEntries is gone, which also disposes of your M10 (it carried no information the line above it did not already have).

Pinned at the loader seam over a real OpenCode database with your scenario — a pane holding [msg_1, msg_4, msg_5] receiving [msg_3, msg_4, msg_5] — asserting both the order (msg_1, msg_3, msg_4, msg_5) and that the marker stays msg_1. Under the old pair the marker becomes msg_3. Your candidate placed.entries[0] === current.entries[0] is the same rule; putting it inside the function is what lets the test see it.

2. The item-2 rule was a guess presented as a derivation (MEDIUM) — valid, and the timestamps went in

The two families are real and the anchored path could not tell them apart. It can now: the flush stops at the first retained row strictly newer than the fresh entry.

window [a,c]   chunk [b, *c]         a < b  → flush a, then b        ✓
window [b,d,e] chunk [*b, c, *e, g]  d > c  → b, c, d, e, g          ✓

Both families are tests. When either side is undated the comparison is false and the row is flushed — the pre-timestamp behaviour, and the better default by your own fuzz (12,275 to 1,397), which is quoted in the comment so the next reader knows the trade was measured rather than assumed.

On the trimmed-anchor test: you are right and the premise is the proof. A trim drops entries[0..cut) — the oldest side — so every trimmed uuid is older than every retained row, which made b < a < c while the assertion wanted c first. Rewritten with consistent timestamps, and the file now states the rule that would have caught it: every placement is authored in durable order, or the test is about nothing.

3. The "equivalent mutant" claim was false (MEDIUM) — valid, corrected and pinned

The conclusion did not follow from the premise: a stale index makes the caller's flush loop a no-op, but skipping it lets the scan continue to a later anchor that does flush. Your [b,a,x,c] / [*a, f, *b, *c] example is now in the comment and in a test. The comment says what it used to claim, so the next reader learns the error rather than inheriting a silently-edited assertion.

4. Survivors (LOW) — all closed or stated

survivor now
M7 index < next finding 3 — killed
M9 scan from at equivalent, agreed, untouched
M10 addedFreshEntries gone with finding 1
M11 initialOldestMarker !== null genuinely equivalent, and the comment now says why: marker and offset are assigned together, so a null marker implies a null offset, and keepWindowCursor can only be false with a null marker when the window was empty — where current.historyOldestOffset is null too. It stays because it states the invariant the two lines share.

Two more I found closing the battery: the empty-window guard on keptWindowHead, and a tie in the new comparison (equal timestamps are not strictly newer, so they flush — the same convention isStrictlyNewer uses on the unanchored path). Both now have tests.

The historyCursorRefresh fixture slices offsets alongside entries through one helper, with a comment naming why it mattered even though no provider emits offsets today.

Full battery, ten mutations, no survivors: the old cursor pair, keptWindowHead forced true and false, the empty-window guard, dropping the timestamp bound, reverting the flush entirely, the stale-anchor guard, a tie, undated treated as newer, and flushing past the anchor.

5. The export's justification (LOW) — valid, rewritten

"The loader would hide it" is not true — the cursor half of this PR tests through that seam successfully. The comment now gives the real reason (the interesting chunk/window pairs take a specific paging history to stage) and names finding 2 as this export's cost, so the trade is visible rather than asserted.

6. What held up

The tail-consumer fuzz (tailMoved: 0 over 65k cases), the no-drop/no-duplicate proof, the [b,a,c] reproduction, the permanence check, and confirming item 3's direction are all evidence the PR should have carried. The [e,f,a,b,c,d,g,h] block is rewritten and now says "a tail the window already holds" in the sentence that describes it, not two sentences above.

On what you could not confirm: I did not find a production path for finding 2's second family either, and the rule is now correct for both regardless — which is the point of using the timestamps rather than choosing a family. The stale-anchor state (finding 3) is reachable only from a window this PR exists to stop creating, and the comment says exactly that.


npx tsc -b clean on Node 24. --project unit --project renderer src/renderer/src/workspace/hook/: 274 passed (45 files).

@Juliusolsson05
Juliusolsson05 merged commit 8cb3331 into main Sep 20, 2026
2 checks passed
@Juliusolsson05
Juliusolsson05 deleted the fix/history-merge-order branch September 20, 2026 15:19
Juliusolsson05 added a commit that referenced this pull request Sep 20, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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