fix(corekit): keep SeekableReader's buffer content when truncating it (#622)

[JarryShaw]

Fixes #622.

SeekableReader.truncate kept padding in preference to content, and read then returned one octet fewer than the buffer held. Two mechanisms, not one — which is why this is not the blind rjust → ljust swap #604 was.

Reproduction, re-verified on current main

pcapkit/corekit/io.py:328 as the issue says, unmoved. Measured against an immutable git archive snapshot, with the editable install's _EditableFinder stripped from sys.meta_path (a meta-path finder runs before sys.path, so neither PYTHONPATH nor PYTHONSAFEPATH=1 can displace it) and pcapkit.__file__ asserted:

pcapkit.corekit.io.__file__: /tmp/io622/before/pcapkit/corekit/io.py
sha256(io.py)              : 919e492381ddb1628c014a717d4c7637702ba8e0c3a399d4a201d594f7c91973

    read(4)      -> b'abcd'
    truncate(8)  -> 8
    seek(0)      -> 0
    read(8)      -> b'\x00\x00\x00e' (4 octets, 8 requested)

After: b'abcde', 5 octets. Five is the whole of what a five-octet stream can answer an eight-octet request with, so five is correct rather than still-short.

The two mechanisms, isolated by measurement

Four trees, each an immutable chmod -R a-w snapshot:

tree	sha256(pcapkit/corekit/io.py)	the repro above
base, untouched	919e4923…	b'\x00\x00\x00e' — 4 octets, content lost
only the truncate change	9e1353d8…	b'abce' — 4 octets, content restored bar the dropped d
only the read change	46bb8fd8…	b'\x00\x00\x00\x00e' — 5 octets, content still all padding
both	—	b'abcde' — 5 octets, correct

Orthogonal, which is the point: the read cap governs how many octets come back (4 → 5), truncate governs which octets they are (padding → data). Neither alone fixes the reported output.

truncate — io.py:331 for the reported symptom, io.py:328 for its twin. truncate(8) on a default 8192-octet buffer is a reduction, so it took line 331, temp[-size:]. Note what that slices: the buffer, not the content. The buffer is _buffer_size long but only [0:_buffer_cur] of it was ever read, so taking its tail keeps eight octets of padding and throws abcd away. The line the issue names, 328's temp.rjust(size, b'\x00'), is the extension path and is not what produces the reported output — it needs a buffer smaller than the requested size to reach:

	buffer after read(4), truncate(8)	read(4)
before, buffer_size=4	b'\x00\x00\x00\x00abcd'	b'\x00\x00\x00e'
after	b'abcd\x00\x00\x00\x00'	b'abcd'

read — io.py:375. min(size, self._buffer_cur - 1) is a count less one, measured from the start of the buffer rather than from the position being read from. At the start of the buffer it is one octet short, and the shortfall was then made up from the stream past the octet it had skipped — dropping it and leaving the return short — while anywhere further in it reaches beyond the content and hands the padding back as data. Independent of truncate entirely:

    read(4)      -> b'abcd'
    seek(0)      -> 0
    read(5)      -> b'abce' (4 octets, 5 requested)      # before: d dropped
    read(5)      -> b'abcde' (5 octets, 5 requested)     # after

What io.IOBase.truncate's contract says, and where it stops

Resize the stream to the given size in bytes (or the current position if size is not specified). The current stream position isn't changed. This resizing can extend or reduce the current file size. In case of extension, the contents of the new file area depend on the platform (on most systems, additional bytes are zero-filled). The new file size is returned.

— https://docs.python.org/3/library/io.html#io.IOBase.truncate

The extension is settled by that on its own terms rather than by analogy with #604: the new file area is the region past the old end, so its zeros belong at the tail. Three further clauses were also being broken:

• "The current stream position isn't changed" — it was. Rebuilding io.BytesIO resets the position to 0 and nothing put it back. Measured: with the buffer position at 2, truncate(8) (to the size it already had, so the content is byte-identical either way and only the position can account for the difference) left it at 0, and the next read(2) returned b'ab' where it should return b'cd'. Now restored, shifted by however far the window's base moved so that it still denotes the same octet. A position past the new end is left where it is, as io.BytesIO leaves it (measured on 3.14.7: truncate(2) with tell() at 4 leaves 4).
• "or the current position if size is not specified" — size is None was hardcoded to 0, discarding the whole buffer. Now the current position, relative to _buffer_set.
• The return value was already the new size and is unchanged.

What a reduction keeps

This is where the io contract stops being decisive, and getting it wrong is how the first revision of this PR introduced a silent-corruption regression. The contract describes a stream whose octet 0 is at absolute offset 0. This buffer is a sliding window over a stream that cannot be seeked: its octet 0 sits at absolute _buffer_set, and _write_buffer already slides it forward on overflow, keeping the most recent octets and advancing _buffer_set to match.

So a reduction keeps the most recent size octets of the content and advances _buffer_set past the ones it drops. What this holds is lookback, and the octets it can still answer for are the ones just read — the stream is already beyond them either way.

That is not cosmetic. _buffer_set + _buffer_cur is what seek reads as "how far the stream has been consumed", and therefore as its licence to read ahead and fill a gap. Advancing the base keeps that sum exact — the base gains precisely what the content pointer loses, so it is unchanged by construction for every size. The first revision of this PR clamped _buffer_cur without moving the base, which left the sum short of the stream, and the next forward seek then spliced in octets from the wrong absolute offset and said nothing:

r = SeekableReader(io.BytesIO(b'abcdefghijklmnop'), buffer_size=8)
r.read(8)      # b'abcdefgh'
r.truncate(3)
r.seek(6)
r.read(1)      # base:  ValueError (loud)
               # rev 1: b'l'  <- silently wrong; offset 6 holds b'g'
               # now:   b'g'

Worth stating plainly because it is the more instructive half of this PR: on main that sequence crashes, and the first revision turned the crash into silently wrong data, which is worse. Caught by a cross-review, then fixed here. Three positions, one of which is right:

	_buffer_cur ≤ _buffer_size	_buffer_set + _buffer_cur = stream consumption
base	✗ — addresses octets the buffer lacks → ValueError	✓ (accidentally, by changing neither)
revision 1	✓	✗ → silent wrong octets
this revision	✓	✓

Two crashes, both fixed

truncate could leave the reader in a state where the next read raised rather than returning anything — ValueError: memoryview assignment: lvalue and rvalue have different structures, from _write_buffer. Two independent routes:

1. _buffer_cur was not brought down with the buffer it indexes, so it addressed octets a reduced buffer no longer had.
2. truncate(0) produces a zero-length buffer, and _write_buffer's buf[-self._buffer_size:] is buf[-0:] for one — the whole of the octets just read rather than none of them. Now counted from the front, buf[buf_len - self._buffer_size:]. That state is reachable only through truncate: the constructor refuses it, io.BufferedReader rejecting a non-positive buffer_size with ValueError: buffer size must be strictly positive. So it belongs to this fix rather than to a separate one.

BytesIO.seek with SEEK_END was checked before relying on it rather than guarded speculatively: it clamps a negative result to 0 rather than raising (io.BytesIO(b'').seek(-1, io.SEEK_END) → 0), so the seek(-buf_len, SEEK_END) beside that slice needs no guard and did not get one.

Fuzzing, which is what found all three

None of the three came out of reading the code. 3000 random sequences of read/read1/readline/peek/seek/truncate over nine buffer sizes, checked after every operation against three invariants — the buffer's physical length equals _buffer_size; _buffer_cur lies within it; and _buffer_set + _buffer_cur equals the octets the underlying stream has actually handed out, counted by a BytesIO subclass that tallies every read/read1/readline — plus any exception the class does not document:

3000 rounds	base	this PR
bookkeeping left inconsistent	2372	0
undocumented exception raised	683	0

One caveat on that harness, since it bit me: the counter subclass initially overrode read and readline only, and SeekableReader also calls stream.read1, which BytesIO provides natively. That undercounted consumption and produced 9887 phantom invariant breaks until read1 was counted too. The numbers above are from the corrected harness.

Why the inputs discriminate

b'abcde', b'abcdefghijkl' and b'abcdefghijklmnop' are non-zero and non-uniform, so head and tail handling cannot produce the same bytes: growing abcd to eight gives abcd\x00\x00\x00\x00 one way round and \x00\x00\x00\x00abcd the other, and reducing abcdefgh to three gives fgh, abc or def depending on which end and which accounting — all distinguishable. The table-driven test uses sizes 3, 5, 7 and 8 rather than all multiples of one number, so an implementation off by a constant cannot pass it; the 7 case distinguishes b'abcde\x00\x00' from b'bcde\x00\x00\x00' by a single octet's shift. It carries one control (buffer_size=8, size=8) whose expected value is identical either way, which passes on both sides, and it asserts _buffer_set + _buffer_cur against the octets each case took off the stream.

The position test asserts the return value, tell(), the buffer's own position, and the bytes read, since the contract covers all of them.

Why nothing caught any of it

• The existing truncate assertions checked the return value and never the content, exactly as the issue says.
• The existing buffered-read round trip at test_buffered_readline_read_read1_and_peek_paths seeks to 1, where _buffer_cur - 1 numerically equals the octets actually available — measured, 2 and 2 — so the wrong cap and the right one agree there. Seeking to 0 separates them.

Evidence

Both runs from immutable chmod -R a-w git archive snapshots, the same test file on each side, exit codes read from a file rather than a pipeline.

Unfixed io.py (919e4923), new tests — 13 failed, 14 passed, 5 subtests failed, exit 1:

FAILED ...::test_buffered_read_returns_every_buffered_octet
FAILED ...::test_seek_variants_warnings_and_truncate_sizes
FAILED ...::test_truncate_below_the_content_leaves_the_reader_usable
FAILED ...::test_truncate_keeps_the_leading_octets_and_pads_the_tail
FAILED ...::test_truncate_keeps_the_window_base_in_step_with_the_stream
FAILED ...::test_truncate_leaves_the_position_where_it_was
SUBFAILED(buffer_size=4, read_size=4, size=8) ...::test_truncate_pads_and_keeps_on_the_side_the_bookkeeping_expects
SUBFAILED(buffer_size=4, read_size=4, size=3) ...
SUBFAILED(buffer_size=8, read_size=6, size=3) ...
SUBFAILED(buffer_size=8, read_size=3, size=5) ...
SUBFAILED(buffer_size=8, read_size=5, size=7) ...
FAILED ...::test_truncate_to_nothing_leaves_the_reader_usable
FAILED ...::test_truncate_without_a_size_resizes_to_the_current_position
13 failed, 14 passed, 3 warnings, 1 subtests passed in 0.58s
REAL EXIT CODE: 1

Fixed io.py (f1141f91), same tests:

22 passed, 2 warnings, 6 subtests passed in 0.46s
REAL EXIT CODE: 0

Wider, in the worktree itself: tests/corekit 167 passed / 378 subtests / exit 0, and tests/foundation/test_extraction.py 12 passed / 28 subtests / exit 0 — that file is the one place in the suite that wraps a non-seekable stream in SeekableReader, so it is the regression surface for the read change. util/changelog_md.py --check exits 0.

Coverage cannot be the evidence axis here, because pcapkit/corekit/io.py was already at 100% statement and 100% branch before this change — the edited lines already executed. It holds at 100%/100% after (211 statements / 80 branches → 215 / 78). The axis is the test count: 14 tests → 22, 0 subtests → 6, with 13 of the 22 failing without the fix.

No fixture is involved. Every case runs against io.BytesIO, so this suite needs nothing from examples/captures.

Reachability

Latent rather than live: grep -rn '\.truncate(' pcapkit/ returns nothing, so no caller in the library reaches it — my own grep, confirming what #593's review recorded. It is public on a public class, and wrong when a consumer calls it.

Found and deliberately not fixed

• A refused seek leaves _tell already moved, and the next read then returns the wrong octet silently. seek assigns self._tell = offset before the check that rejects the offset, so SeekError escapes with the position already changed. Measured identically on the base and on this branch:

  r = SeekableReader(io.BytesIO(b'ABCDEFGHIJKLMNOPQRSTUVWXYZ'), buffer_size=5)
  r.read(16)     # b'ABCDEFGHIJKLMNOP'; _buffer_set = 11, _buffer_cur = 5
  r.seek(8)      # SeekError: cannot seek before the beginning of the buffer: 8 < 11
  r.tell()       # 8  <- moved, though the seek was refused
  r.read(1)      # b'L' -- DATA[11], the buffer's start; DATA[8] is b'I'
  

  Not reachable from truncate and not what SeekableReader.truncate pads on the wrong side and returns fewer octets than requested #622 reports, so it is left for its own issue. It is the largest remaining source of divergence when the fuzz above is also compared against a BytesIO oracle: read/seek-only sequences that disagree fall from 27 to 14 of 3000 with this PR, and the residue is this. A related one alongside it: the SEEK_CUR and SEEK_END branches never check that the resulting _tell is non-negative at all, where SEEK_SET at least validates its raw offset.

• The same min(size, self._buffer_cur - 1) cap appears three more times — in readline, read1 and peek. Only read's copy is fixed here, because it is the one the reported symptom runs through; readline's interacts with its newline semantics and each needs its own discriminating test. None is reachable from truncate or seek: seek's internal read1 call sets _tell to the end of the buffered content first, which takes the stream branch. Worth flagging for whoever picks them up that read and peek carry byte-identical copies of the line, so a search-and-replace will hit two sites that need different treatment.

• _buffer's position can drift out of step with _tell without a seek to resync it, because peek writes into the buffer without advancing _tell. A read from that state reads from the stale position. Not fixed, and this change cannot make it worse: the new cap is never larger than the old one whenever the position is past the buffer start, so it leaks less padding, not more.

• writeable() returns False, and io's own contract says a non-writable stream's truncate raises — io.BufferedReader.truncate over a read-only file raises io.UnsupportedOperation('truncate'), measured on 3.14.7. This class deliberately reinterprets truncate as resizing the buffer instead. Making it raise is an API break and a call for the owner, so it keeps its existing semantics; the docstring now says plainly that what it resizes is the buffer and not the stream behind it.

• Octets dropped by a reduction cannot be recovered, the stream not being seekable. A position left among them is then before the window, which seek refuses as it refuses any other. Documented in the method's Note: rather than worked around.

[JarryShaw]

The two mechanisms, isolated by measurement

The description asserts that fixing truncate alone still returns b'abce'. That was a trace, so here it is measured instead. Four trees, each an immutable chmod -R a-w snapshot, the editable-install _EditableFinder stripped from sys.meta_path and pcapkit.__file__ asserted against the tree in every run:

tree	sha256(pcapkit/corekit/io.py)	read(4), truncate(8), seek(0), read(8) over b'abcde'
ead73b204, untouched	919e4923…	b'\x00\x00\x00e' — 4 octets, content lost
only the truncate change	9e1353d8…	b'abce' — 4 octets, content restored bar the dropped d
only the read change	46bb8fd8…	b'\x00\x00\x00\x00e' — 5 octets, content still all padding
both (this PR)	83cc1318…	b'abcde' — 5 octets, correct

The two halves are orthogonal, which is the point: the read cap governs how many octets come back (4 → 5), and the truncate padding side governs which octets they are (padding → data). Neither half alone fixes the reported output, and the failure modes do not overlap — so this could not have been the one-character rjust → ljust swap that #604 was.

The half-fixed trees were derived from the two endpoint snapshots by text substitution rather than by hand-editing, with an assertion that exactly one occurrence was replaced. Worth recording why that assertion mattered: read and peek carry byte-identical copies of the old line, buf = self._buffer.read(min(size, self._buffer_cur - 1)), so matching on the line alone hits two sites. read's is identified by its trailing size_rem = -1. peek's copy is one of the three left unfixed and listed in the description.

[JarryShaw]

Amended: a second crash route, found by fuzzing after the first push

The commit was amended (489ab203d → 79730f7b1) and the description rewritten. What changed and why:

Fuzzing 3000 random operation sequences against the buffer's own invariants showed the _buffer_cur clamp closed 2372 of 2372 inconsistent-bookkeeping rounds but only 237 of 683 rounds that raised an undocumented exception. The residue was a second route to the same ValueError: memoryview assignment: lvalue and rvalue have different structures:

r = SeekableReader(io.BytesIO(b'abcde'), buffer_size=5)
r.truncate(0)
r.read(1)      # ValueError, before and after the first push

_write_buffer does self._buffer_view[:] = buf[-self._buffer_size:], and for a buffer of no size at all buf[-0:] is buf[0:] — the whole of the octets just read rather than none of them — assigned into a view with no room. Counting from the front instead (buf[buf_len - self._buffer_size:]) is correct at every size and is what the amended commit does.

This belongs in this PR rather than a separate one because truncate is the only way to reach a zero-length buffer: the constructor refuses one, since io.BufferedReader rejects a non-positive buffer_size with ValueError: buffer size must be strictly positive. Leaving it would have shipped "truncate now leaves the reader usable, except at size 0, where it still crashes".

3000 fuzz rounds	ead73b204	first push	amended
bookkeeping left inconsistent	2372	0	0
undocumented exception raised	683	446	0

One thing deliberately not changed while in there: the self._buffer.seek(-buf_len, io.SEEK_END) two lines below looks like it should go negative on the same input, but BytesIO.seek with SEEK_END clamps a negative result to 0 rather than raising — measured, io.BytesIO(b'').seek(-1, io.SEEK_END) returns 0 — so it needs no guard and did not get a spurious one.

A seventh test covers it, test_truncate_to_nothing_leaves_the_reader_usable, asserting that reads still work forward afterwards and that seeking back fails — with nothing buffered there is no lookback, so SeekError is the right answer there and is what it gives.

The fuzz also turned up a pre-existing defect that is not fixed here — a refused seek leaves _tell already moved, so the next read silently returns the wrong octet. It is measured identically before and after this PR, is not reachable from truncate, and is written up in the description's "found and deliberately not fixed" section with its reproducing sequence.

[JarryShaw]

Cross-review verdict: NEEDS CHANGES (now addressed)

Per the standing rule that an agent-raised change gets reviewed by a different model, this PR was cross-reviewed by a Sonnet subagent (the PR was authored by Opus), briefed to falsify rather than bless — a verdict per claim, with evidence it obtained itself, and told that disagreements were the valuable output. It ran read-only against its own immutable snapshots under /tmp/io622review/, with the editable-install sys.meta_path finder stripped and pcapkit.__file__ asserted before every measurement. It verified the three commit hashes it was given against what it measured.

It returned NEEDS CHANGES on two counts. Both are now fixed, in 50e8c1d40.

1. The reduction desynced the window base from the stream — silent wrong octets

SeekableReader's truncate() reduction still desyncs the buffer's position bookkeeping from the raw stream's true consumption point, and a subsequent forward seek() silently splices in bytes from the wrong absolute stream offset — no exception, just wrong octets. […] silent data corruption is a materially different and worse claim than "gone for good", which is what the current docstring says.

r = SeekableReader(io.BytesIO(b'abcdefghijklmnop'), buffer_size=8)
r.read(8); r.truncate(3); r.seek(6)
r.read(1)   # -> b'l', should be b'g'

Upheld, and worse than reported. I reproduced it, then measured the same sequence on the base commit, which the review had not: there it raises ValueError: memoryview assignment: lvalue and rvalue have different structures. So the previous revision of this PR converted a loud crash into silently wrong data — my regression, not a pre-existing condition.

Root cause, as the review diagnosed it: _buffer_set + _buffer_cur is what seek reads as "how far the stream has been consumed", and hence as its licence to read ahead. Clamping _buffer_cur made it honest about the buffer while making that sum dishonest about the stream.

Fixed by treating the buffer as what it is — a sliding window whose octet 0 sits at absolute _buffer_set. A reduction now keeps the most recent size octets of the content and advances _buffer_set past the ones it drops, so the base gains exactly what the content pointer loses and the sum is unchanged by construction, for every size. The repro now returns b'g'. Full reasoning is in the description under "What a reduction keeps", including a table of the three positions and why only the third holds both invariants.

Two things came with the fix: a regression test, test_truncate_keeps_the_window_base_in_step_with_the_stream, and a third fuzz invariant — that _buffer_set + _buffer_cur equals the octets the underlying stream has actually handed out, counted by a BytesIO subclass that tallies them. That invariant now holds in all 3000 rounds; it did not before. The review's own fuzz had flagged the same area through a different proxy (_buffer.tell() > len(buffer), 2397/3000), which is why the finding is worth the credit — though that particular proxy is not itself a defect, since a position past the end is legal for BytesIO and is exactly what the position-preservation clause asks for.

2. A stale number in the commit message

tests/corekit is reported as "165 passed / 378 subtests", but […] the real total is 166 passed, not 165. 378 subtests is right. 165 is the old (pre-amendment) figure that didn't get bumped.

Upheld. 165 was correct when written and went stale when the 7th test landed. Re-measured directly rather than adopted: tests/corekit is now 167 passed / 378 subtests / exit 0, the 8th test having landed since. Corrected in the commit message and the description.

What the review confirmed

Claims 1–6 and 8–11 CONFIRMED, each with independently obtained evidence: the reproduction both ways; that b'abcde' is a consistent end state rather than still-short; the mechanism separation, which it re-derived by building its own truncate-only and read-only trees and got my exact counterfactuals (b'abce' and b'\x00\x00\x00\x00e'); that the repro takes the reduction branch and never line 328, which it established by instrumenting the branch rather than by reading; the io.IOBase.truncate contract quotation, verbatim from python.org, and the BytesIO position behaviour on 3.14.7; the derivation of _buffer_set = 2, _tell = 6 behind the changed truncate(None) assertion, traced by hand through _write_buffer's reshuffle arithmetic; that buf_rem can be neither negative nor larger than the buffer, proved analytically from the branch's entry condition and unfalsified across ~9000 fuzz trials; that the position test's content really is byte-identical on both sides, so it isolates position from content as claimed; and the coverage figures to the statement.

On the amendment it also proved the buf[-k:] → buf[buf_len - k:] equivalence for every k ≥ 1 rather than spot-checking it, confirmed the constructor is the only other route to _buffer_size and refuses 0, and independently measured that BytesIO.seek with SEEK_END clamps rather than raising.

On what stays unfixed

It confirmed the separately disclosed seek defect — a refused seek leaving _tell moved — reproduces byte-for-byte identically on the base and on this branch, and agreed that leaving it out is defensible: pre-existing, orthogonal, not entangled with the read cap change. It found one more of the same family unprompted, which is now disclosed in the description too: the SEEK_CUR and SEEK_END branches never check that the resulting _tell is non-negative at all.

Both want their own issue. I have not filed one, not having been asked to create issues here.

[JarryShaw]

One follow-up amend, 50e8c1d40 → 478296afe, docs only — no change to pcapkit/corekit/io.py, whose sha256 stays f1141f91….

test_truncate_keeps_the_leading_octets_and_pads_the_tail said the defect was keeping the trailing size octets. That was true of the first revision's reading and is not true of this one: a reduction below the content now deliberately keeps trailing octets — of the content. What the old code sliced was the buffer's tail, which is padding wherever the buffer is not full. The test is renamed test_truncate_keeps_the_content_and_not_the_padding and its docstring restates the distinction, since a docstring contradicting the rule it documents is how a stale expectation survives (the lesson of #627).

Re-measured after the rename: 13 failed / 14 passed / 5 subtests failed / exit 1 on the base, 22 passed / 6 subtests passed / exit 0 with the fix, util/changelog_md.py --check exit 0.

Repository CI is queued rather than running — every workflow run in the repo is queued, with eight PRs open, so this is a runner backlog and not a result on this branch.