fix(corekit): refuse truncate on a reader that reports itself unwritable (#645)

[JarryShaw]

Fixes #645. Owner decision on that issue, in one word: "fix it" — the API break is accepted.

What was wrong

io.IOBase.writable gates two methods, and says so in the one place both implementations agree:

Return a bool indicating whether object was opened for writing.

If False, write() and truncate() will raise OSError.

That is _pyio.py:451-456 on CPython 3.14.7 (/home/linuxbrew/.linuxbrew/opt/python@3.14/lib/python3.14/_pyio.py on the machine this was measured on — line numbers re-read here, not taken from the issue), with _checkWritable at 458-463 and _BufferedIOMixin.truncate — the base every buffered reader in _pyio inherits, and neither writable nor truncate is overridden by _pyio.BufferedReader — at 791-804.

SeekableReader honoured that gate for two of the three methods it covers and not for the third:

r = SeekableReader(io.BytesIO(b'hello world'), buffer_size=8)
r.writable()          # False
r.write(b'x')         # UnsupportedOperation: write     <- honours the gate
r.writelines([b'x'])  # UnsupportedOperation: write     <- honours the gate
r.truncate()          # 0                              <- returns
r.truncate(4)         # 4                              <- returns

So a caller that checked writable() before calling truncate — which is exactly what the contract invites — got a surprise either way round. And this is not a pure-Python nicety the accelerated path skips: every ordinary read-only file object in CPython raises here, measured rather than recalled.

f = open(path, 'rb')          # <class '_io.BufferedReader'>
f.writable()                  # False
f.truncate()                  # io.UnsupportedOperation: truncate

Separately, and named in the same issue: the writability override in this file was spelled writeable, which is not how the io protocol spells it, so it overrode nothing.

'writeable' in SeekableReader.__dict__          -> True
'writable'  in SeekableReader.__dict__          -> False
SeekableReader.writable is io.IOBase.writable   -> True

io, shutil and any third-party caller therefore read the inherited IOBase.writable and never saw the method defined here. Both returned False, which is the coincidence that hid it — there was no symptom, and editing writeable would silently have had no effect.

Was writable() honest?

Yes, and this is worth stating because the two halves of the issue could have disagreed and did not. writable() resolved to IOBase.writable and returned False; the reader genuinely cannot write, since write raises. So the reporting was never the dishonest half — the missing guard on truncate was, and the misspelling was about where the method was defined, not about what it answered. The value this PR reports is unchanged. Had writable() been returning True over an unwritable stream, the guard and the reporting would both have needed changing; that is not the case here.

What changed

• truncate() raises UnsupportedOperation('truncate'), unconditionally. Unconditional rather than if not self.writable() because write and writelines are unconditional too — this reader is never writable, and a guard whose false branch cannot execute is a dead branch with no coverage.
• The resizing is not deleted, only made private, as _truncate_buffer. It never touched the underlying stream — it resizes a private lookback window, which is precisely the counter-argument the issue raised — and it is the only route to the "position dropped by the window" state that seek and the four buffered read paths must refuse. SeekableReader's position bookkeeping is not maintained across operations: a refused seek leaves _tell moved, SEEK_CUR/SEEK_END can drive it negative, and peek desyncs the buffer cursor #643 and The four buffered read paths cap size with min(size, self._buffer_cur - 1): a count-less-one measured from the buffer's start instead of the read position #644 landed tests for those refusals; deleting the mechanism would take their mechanism with them and make _seek_buffer's documented SeekError branch unreachable. It has no production callers, and had none as a public method either (grep -rn '\.truncate(' pcapkit/ → nothing), so this narrows exposure rather than removing capability.
• writeable() → writable(). Same body, same False, the spelling the protocol uses. Not kept as an alias: an alias would keep a name that answers nothing in the public API forever, and the issue's point is that the suite had locked the misspelling in.
• docs/source/pcapkit/corekit/io.rst follows the rename. _truncate_buffer is deliberately not added there, matching _write_buffer and _seek_buffer, which are also undocumented.

The exception type, argued rather than assumed

The contract names OSError, a builtin, and the house rule is in-library exceptions for in-library errors. Here there is no trade-off to make, because the two coincide by inheritance:

pcapkit.utilities.exceptions.UnsupportedOperation.__mro__
  -> UnsupportedOperation, BaseError, io.UnsupportedOperation, OSError, ValueError, Exception, ...

pcapkit's UnsupportedOperation (pcapkit/utilities/exceptions.py:477) subclasses io.UnsupportedOperation, which subclasses OSError. So raising the in-library exception satisfies the stdlib contract literally — isinstance(exc, OSError) is True, and isinstance(exc, io.UnsupportedOperation) is True, which is what makes the ABC-equivalence assertion below pass. Raising a bare OSError instead would have been strictly weaker (it loses BaseError's behaviour and the narrower type) for no gain in conformance. It is also what write and writelines in this same class already raise, and 'truncate' is the exact message CPython's C BufferedReader carries.

Assertion against the ABC, not just the behaviour

test_truncate_refuses_with_the_same_exception_the_stdlib_reader_raises does not name the expected type. It captures it by running the same call on a real read-only file object, then asserts SeekableReader raises that captured type:

with open(path, 'rb') as accelerated:          # a C io.BufferedReader
    with self.assertRaises(OSError) as caught:
        accelerated.truncate()
    baselines['io.BufferedReader'] = type(caught.exception)
...
with self.assertRaises(expected):
    reader.truncate()

Both implementations are baselined — the accelerated io.BufferedReader and _pyio.BufferedReader over a non-writable raw stream. Their messages differ (truncate versus File or stream is not writable.) and their type does not, which is why the type is what is asserted. Capturing rather than naming it is also what keeps the assertion valid across the 3.10–3.15 matrix: the assertRaises(OSError) around the baseline call is what enforces the one assumption being made, so a CPython version that raised something outside the OSError lattice would fail on the stdlib call with a self-describing message rather than mysteriously on ours.

Evidence

Exit codes read from files, never from a pipeline. Tree asserted before every measurement — pcapkit.__file__ = /local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a728a1132f06428d8/pcapkit/__init__.py, with the editable-install finder stripped by getattr(f, '__module__', '') (the finders at sys.meta_path[1:5] are classes, so a filter on type(f).__name__ strips nothing).

The three new tests on b34f132f6, this PR's base — pcapkit/corekit/io.py restored to HEAD and the new tests run against it:

7 failed, 2 passed, 31 deselected in 0.17s

exit code 1. The failures, in full:

SUBFAILED(args=())        test_truncate_refuses_on_a_stream_that_reports_itself_unwritable
SUBFAILED(args=(0,))      test_truncate_refuses_on_a_stream_that_reports_itself_unwritable
SUBFAILED(args=(4,))      test_truncate_refuses_on_a_stream_that_reports_itself_unwritable
SUBFAILED(args=(None,))   test_truncate_refuses_on_a_stream_that_reports_itself_unwritable
SUBFAILED(baseline='io.BufferedReader', expected='UnsupportedOperation')
                          test_truncate_refuses_with_the_same_exception_the_stdlib_reader_raises
SUBFAILED(baseline='_pyio.BufferedReader', expected='UnsupportedOperation')
                          test_truncate_refuses_with_the_same_exception_the_stdlib_reader_raises
FAILED                    test_writable_is_the_method_the_io_protocol_consults

AssertionError: UnsupportedOperation not raised for the first six; AssertionError: 'writable' not found in mappingproxy({...'truncate': ..., 'writeable': ...}) for the last, whose dump is itself the evidence that writeable was there and writable was not. Note the "2 passed" on the same line as the failures: those two per-test lines report PASSED while every subtest under them SUBFAILED, so the exit code and the subtest tally are the only trustworthy signal.

The same three tests here:

3 passed, 31 deselected, 6 subtests passed in 0.04s

exit code 0. Six subtests passed where six SUBFAILED.

Coverage did not go backwards. coverage run -m pytest tests/corekit/test_io.py, --include='pcapkit/corekit/io.py', branch coverage on:

	Stmts	Miss	Branch	BrPart	Cover	tests	subtests
base b34f132f6	235	0	88	0	100%	31	35
this PR	237	0	88	0	100%	34	41

Both exit 0. The changed lines already executed on the base — writable's return False and every line of the resize mechanism — so per the standing rule the subtest count is quoted instead: 35 → 41, and 31 → 34 tests.

Scope run: tests/corekit/ — 179 passed, 4 warnings, 413 subtests passed in 158.48s, exit code 0. The full suite was deliberately not run (it has peaked at 41.4 GB RSS on this host).

EXPECTED_FAILURES in tests/protocols/test_option_roundtrip_unit.py — imported rather than grepped, since ** unpacking hides the entries from a grep. 45 entries, unchanged: nothing in that path calls truncate, writeable or writable, confirmed by grep -rn '\.truncate(\|\.writeable(\|\.writable(' pcapkit/, which returns nothing outside io.py's own definitions.

Version sensitivity: _pyio is in sys.stdlib_module_names and present on every CPython in the matrix; ast.parse(..., feature_version=) accepts both changed files at 3.10, 3.11, 3.12 and 3.13, so nothing here needs a floor above the current one. Only 3.14.7 was available locally to run, which is why the ABC baseline is captured at runtime rather than hardcoded — that is the part a single interpreter could not otherwise vouch for.

Labels

fix + breaking.

fix because this corrects a documented-contract violation rather than adding a capability; the subject prefix is fix:, which is what the type labels key on. breaking because this raises where it used to succeed, in two separate ways: truncate() now raises UnsupportedOperation instead of returning a size, and writeable() is gone, so an external caller of either gets an exception where they used to get a value. Nothing inside the repository breaks — truncate had no production callers and neither spelling of the writability method was called anywhere in pcapkit/ — so the break is entirely about external callers, which is exactly the risk the issue was raised to get a decision on. docs does not apply (it is scoped to documentation-only changes) and neither does test (the subject prefix is fix:).

Not claimed

CI is not claimed green — the queue is deep, and a check count of "1 pass" on this repository is pyup.io/safety-ci, a StatusContext rather than an Actions job. The changelog bullet for this is not on this branch; it goes to #657 (docs/changelog-1.5.0) separately, per the standing rule that no changelog file is touched on a feature branch.

[JarryShaw]

Supplementary measurement: does the Python-level writable() override change anything?

The PR body reasoned about this and did not measure it, which is the weakest part of the argument as posted, so here is the measurement.

Before this change, SeekableReader.writable resolved to the C io.IOBase.writable. After it, a Python method shadows that name on a subclass of the C io.BufferedReader. If any C-level or stdlib path consulted it, the rename could have an unintended consequence beyond the two deliberate breaks.

Measured on the PR head (83e06ec82), tree asserted — pcapkit.__file__ = /local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a728a1132f06428d8/pcapkit/__init__.py, editable-install finder stripped:

--- does any ordinary operation consult writable()? ---
writable() call count during read/seek/peek/readinto/flush: 0
--- shutil.copyfileobj out of a SeekableReader ---
copied: b'abcdefghijklmnop'
--- the C reader constructed over it still reports the same as before ---
writable(): False
io.IOBase.writable(r2) (the value the base would have given): False
readable(): True seekable(): True
truncate() -> pcapkit.utilities.exceptions.UnsupportedOperation | truncate
write()    -> pcapkit.utilities.exceptions.UnsupportedOperation | write
writeable still present? False
--- and the private mechanism still works, from the private name only ---
_truncate_buffer(3) -> 3
buffer now: b'def' _buffer_set = 3
read(1) after -> b'g'

The call count comes from a subclass whose writable() appends to a list before delegating, driven through read, peek, read1, readline, seek, readinto, readinto1, tell, readable, seekable, isatty and flush. Zero — nothing in the C buffered-reader machinery consults the Python method, which is consistent with the C implementation reading its own self->writable flag set at construction from the raw stream rather than calling back into Python. io.IOBase.writable(r2) returns the same False the override returns, so the override is value-identical to the thing it now shadows; the only change is that it is finally reachable through the protocol's own spelling.

shutil.copyfileobj is included because it is the most plausible third-party consumer of a read-only buffered reader, and it copies all sixteen octets unchanged.

The last block is the point of keeping _truncate_buffer private rather than deleting it: it still advances the window's base to 3 and the following read(1) returns b'g', the octet actually at offset 6 — the #644 behaviour this PR must not lose.

[JarryShaw]

Cross-review: GOOD TO GO

Independent cross-review by a subagent on a different model (Sonnet), briefed to falsify rather than to bless — a verdict per claim, with evidence it obtained itself, and disagreements treated as the valuable output. No model substitution was needed; Sonnet was the intended model and the one that ran.

It reviewed 83e06ec82 in its own worktree, read-only with respect to this PR, and reproduced every load-bearing claim. All ten confirmed:

#	Claim	Verdict
1	truncate() raises an OSError + io.UnsupportedOperation instance	CONFIRMED
2	writable() is a genuine override, writeable gone	CONFIRMED
3	writable() was already honest on the base	CONFIRMED
4	New tests fail on base (exit 1), pass on head (exit 0)	CONFIRMED
5	Coverage 100% both sides, 235 → 237 stmts, 0 missed	CONFIRMED
6	tests/corekit/ 179 passed / 413 subtests, exit 0	CONFIRMED
7	Keeping _truncate_buffer is justified	CONFIRMED, with a sharper finding — see below
8	No other caller of the changed methods	CONFIRMED
9	EXPECTED_FAILURES unchanged at 45	CONFIRMED
10	The ABC-equivalence test cannot pass vacuously	CONFIRMED

It reproduced the base-versus-head failing/passing runs byte-for-byte, and read its exit codes from a file written by subprocess.call rather than trusting a wrapper — the same trap this PR's own evidence was gathered around.

What it added that the PR did not have

It measured the version boundary on five interpreters, where this PR only had one. On CPython 3.10.21, 3.11.15, 3.12.14, 3.13.15 and 3.14.7 it verified that io.BufferedReader.writable is io.IOBase.writable, that writable/truncate sit at the same IOBase/_BufferedIOMixin level, and that a real open(path, 'rb').truncate() raises UnsupportedOperation: truncate as an OSError + io.UnsupportedOperation on every one of them. That is materially stronger than the single-interpreter measurement in the body above, and it is the claim a single interpreter could not vouch for. 3.15 remains COULD NOT VERIFY — not installed on the host — and is reported as a low residual risk rather than a checked one. 3.15 is experimental: true in unit-tests.yml and cannot block.

It also independently confirmed the _pyio.py line numbers quoted above (writable 451-456, _checkWritable 458-463, _BufferedIOMixin.truncate 791-804, the mixin at 765, BufferedReader at 1019 overriding neither), and confirmed the label set.

What it disputed, and what I did about each

Three findings. One is now fixed; two are declined, with reasons.

1. Fixed — a stale subTest label. tests/corekit/test_io.py:865 labelled its tuple ('truncate', lambda: reader._truncate_buffer(4), 4), so a subTest failure message would have named a method the row does not call. Cosmetic, but it is wrong in exactly the place someone reads when the test fails. Now ('_truncate_buffer', ...). This is the only change since the reviewed commit: head moved 83e06ec82 → c7e42c285, a one-word diff of 1 insertion(+), 1 deletion(-) in a test label. Re-verified after it: 34 passed, 41 subtests passed, pcapkit/corekit/io.py still 237 stmts, 0 miss, 88 branch, 0 BrPart, 100%, exit 0. The verdict above stands on that diff.

2. Declined — -> 'NoReturn' on truncate. A genuine counterpoint, and better evidence than the PR's own reasoning: the repo does use NoReturn for always-raising members (pcapkit/corekit/multidict.py:574, pcapkit/protocols/misc/raw.py:53 and :154, and several in header.py), and mypy.ini sets warn_unreachable = True, which NoReturn would activate for callers. I am keeping -> 'int' anyway, because within io.py the file-local convention is the declared type: write is -> 'int' and writelines is -> 'None', and both always raise. Annotating only truncate as NoReturn would make the file internally inconsistent about its three always-raising methods, and changing write/writelines with it is unrelated churn for Question: truncate() succeeds on SeekableReader although the stream reports it is not writable, and the writeable() override is misspelled so io never consults it #645. The reviewer ran mypy on io.py both ways: zero errors either way (one pre-existing unrelated error at io.py:85, the raw property, present on the base too), so nothing is CI-enforced here. Worth a follow-up pass over all three together; not worth doing to one of them now.

3. Declined — renaming test_seek_variants_warnings_and_truncate_sizes. It now calls _truncate_buffer rather than truncate. The reviewer called this minor and defensible, and I agree: seek variants and the warning are the test's primary subject, and the truncate-adjacent assertions are a tail on it.

The sharper finding on claim 7, recorded honestly

The reviewer confirmed _truncate_buffer is the only site that can push _buffer_set past an already-committed _tell — grep -n "_buffer_set\s*[+]\?=" pcapkit/corekit/io.py gives exactly three assignment sites, and the other two (__init__, _write_buffer) cannot produce the state, _write_buffer's being algebraically bounded. So the justification for keeping the mechanism holds.

But it sharpened the claim in a way worth stating plainly: nothing inside io.py calls _truncate_buffer either, not merely nothing outside it. Its only references in the module are its own def line and three docstrings. So the "position sits before the window" state is currently unreachable from any shipped code path, and the coverage _truncate_buffer preserves is of an internal invariant only the test suite can currently trigger. The PR body says the method has no production callers; it did not spell out that consequence, and it should have. The reviewer's own assessment, which I accept: a legitimate nuance that undersells rather than invalidates, since #643 and #644 are evidence that this bookkeeping is genuinely fragile and pinning the invariant defensively is reasonable even without a live caller.

Also measured since the body was written

tests/foundation/ — the tier that actually exercises SeekableReader through extract() — 235 passed, 11 skipped, 385 subtests passed, exit 0. Worth recording how that run got there: it first came back exit 1 on tests/foundation/reassembly/test_tcp_runtime.py, which looked alarming for a change to the reader the extraction path is built on. The cause was FileNotFoundError: sample capture 'test.pcap' not found — a generated fixture, not git-tracked, absent from a fresh worktree. After python examples/generators/make_samples.py the same test passes (exit 0, 1 passed, 4 subtests) and so does the whole tier. Nothing to do with this change, and recorded here rather than dropped because a reader of the tier's first result would otherwise reasonably suspect it was.

Note also that the task notification for that failing run reported "exit code 0" while the run's own rc file held 1 — the wrapper-versus-pytest exit-code discrepancy, hit live. Every exit code in this PR is read from a file for that reason.