fix(readers): bound the record reads over agent-writable trees

[chenmingwei23]

Problem / Motivation

for line in handle asks the handle for bytes up to the next newline. It carries no
length bound, so a single line with no newline in it is materialised in one allocation
the size of the whole file. That is only a bug when the writer is not trusted -- and for
these readers it is not: every tree involved sits under KiroCrew's data home, the kiro
home, or the crew home, and security.is_sensitive_path fences only the enumerated
leaves in _CREW_SECRET_LEAVES, so an agent's own file tools can write the rest.

PR #6312 closed this for the trash manifest readers and PR #7297 for session_digest's
three transcript scanners. #6345 is the audit of the rest, and it asks for a specific
thing: not a sweep, but a shared bounded reader plus a per-call-site judgement of
whether an over-cap record may be skipped or must abort the read.

This PR delivers that audit. Enumerating the sites turned up two that the issue's own
grep (for line in (fh|f|handle)) had missed, because they spell the loop variable
differently: history_projection.py (for raw in handle) and sel.py:2817
(for raw_line in src_f). The full set is 27 sites, not 26.

Of those, 10 are deliberately left alone with evidence (kernel /proc pseudo-files and
the fenced audit log), 15 are converted here, and one is deliberately deferred to
#7771 -- the snapshot notification merge, for reasons set out below.

Why it matters

The reachability is mundane. Opening the usage dashboard reads the token shards.
Expanding a storage row reads a transcript. Restoring a snapshot reads the live
notifications file. A planted or corrupt multi-GB newline-free line turns any of those
into a file-sized allocation inside the gateway process -- not a degraded panel, an OOM
that takes every session on the host with it.

Two of the converted sites are worse than a crash, because their read decides a durable
write:

• members.read_activity feeds the dedupe_session probe in record_activity, which
  decides whether to append a participation entry or suppress it. A skipped record
  reads as "no prior entry", so the log gets a duplicate, and the participation counts
  that drive trigger generation and select_crew routing are inflated.
• subagent_cost._read_samples has two consumers, and the second one is the dangerous
  one: compact_cost_log parses the log and then os.replaces it with what it parsed.
  A skipped record there is not a lost reading, it is a permanently deleted one.

What changed (motivation -> approach -> change)

Symptom: one hostile line, one file-sized allocation. Root cause: the reader trusts the
writer to insert newlines. Change: jsonl_util -- which already owns the matching bound
on the WRITE side (rotate_jsonl_at) -- gains the read side, and session_digest's
private _bounded_lines is promoted into it so there is one implementation rather than
two copies.

A file's rotation cap does not bound one RECORD, which is why the write bound was not
already enough: rotation only fires when the writer next appends, so a crafted line
lands whole before any rotation sees it.

Four functions, two postures by two forms:

• bounded_records SKIPS an over-cap record, drains its tail, and logs one aggregated
  debug line. For a read that is read-only and degradable.
• strict_records ABORTS by raising OversizedRecord, and deliberately does NOT drain:
  the caller is abandoning the read, so walking a multi-GB line to its end would hand
  the hostile file the cost the cap exists to deny it.
• bounded_raw_records / strict_raw_records are the undecoded twins. The bounded twin
  has one caller: history_projection already iterated a binary handle and prefilters on
  bytes (b'"file_changes"' not in raw) before parsing, so routing it through the
  decoding form would make it decode every record to run a filter that rejects most of
  them. The strict twin has no external caller today -- it is what strict_records is
  built on, and the site that wanted raw strict bytes directly is the deferred merge.
  The module docstring says so, rather than implying a caller that is not there.

The cap is in BYTES and the handles are opened binary for that reason. A character cap
is not a memory bound: one astral code point is four bytes of str under PEP 393, so a
128 MiB character cap admits half a gibibyte of resident text. The reverse holds too --
UTF-8 spends at least as many bytes per code point as CPython's widest string
representation -- so an N-byte read caps the decoded str at N bytes.

One cap serves every caller, sized for the largest record shape any of them reads (a
transcript record carrying a whole conversation turn): MAX_IMAGE_BYTES_PER_MESSAGE is
64 MiB, which base64-expands to ~85 MB, and the largest record measured on a live
install is 77,920,032 bytes. 128 MiB is the smallest round value clearing that. Every
other shape here -- token rows, telemetry export cycles, ~150-byte member activity
entries -- is orders of magnitude smaller, so the shared cap never undercounts them. A
per-format cap would bound each tighter, but the memory that matters is the peak of ONE
record, and a cap below a format's real ceiling buys nothing while risking a silent
undercount. This is also why it is not _MANIFEST_RECORD_CAP (8 MiB): that would
truncate the biggest real sessions.

Peak memory, measured rather than reasoned about

An earlier revision of this branch documented the reader's peak as "roughly twice the
cap". That was wrong, and it was wrong about main too. Measured with tracemalloc at
a 4 MiB cap, on an input that forces a carried tail plus a full read:

reader	peak / cap
main's merged _bounded_lines (PR #7297)	3.03x
this branch, before the fix below	4.03x
this branch now	3.03x

The 4.03x was a real regression of +1x cap over main, and it was mine. Main reads one
line per readline and keeps no carried buffer; this reader must accumulate across reads,
because a \r\n can straddle a read boundary and a bare \r is a boundary. Each read
then asked for a full cap regardless of what the tail already held, so a nearly-full
buffer and a full-size read added up.

Fixed by bounding each read to what the carried record has left:
readline(max(2, cap + 2 - len(buf))). That restores exact parity with main.

cap + 2 is a floor, not a tidy constant. A legal at-cap record ending in a CRLF is
cap + 2 bytes, so a buffer that could only ever hold cap + 1 could not assemble one
and would refuse it -- which is a bug this PR already fixed once. Two spy assertions that
pinned limit <= cap + 1 are relaxed to cap + 2 for that reason, one of them in
test_session_digest.py, which is main's. What those tests exist to pin is that no read
is UNBOUNDED, and that is unchanged; the exact constant was incidental to it, and both
assertions now carry the derivation in a comment so the number does not read as arbitrary.

For the record, one fix that looks obvious does not work: making the buffer a bytearray
so += extends in place instead of allocating the concatenation. Measured at 4.03x,
unchanged. The dominant terms are the buffer and the chunk, not the concatenation.

The site audit, and what each site got

Sites	Tree	Verdict
usage.py x7, telemetry.py, backfill.py x2, history_search.py, history_projection.py, stub.py	data home usage shards, kiro-cli transcripts, telemetry shards, crew-home logs	agent-writable, skip-safe -> bounded_records (13 sites)
members.py, subagent_cost.py	member activity log, cost-sample log	agent-writable, abort-required -> strict_records (2 sites)
snapshot.py x2	notifications.jsonl	agent-writable, abort-required, DEFERRED to #7771 (2 sites)
handlers_system.py x3, platform_compat.py x3, acp/runtime.py, sandbox.py	/proc/meminfo, /proc/net/dev, /proc/locks, /proc/<pid>/status, /proc/<pid>/mountinfo	kernel-synthesised, not agent-writable -> unchanged (8 sites)
sel.py x2	security_events.jsonl / .d	fenced by _CREW_SECRET_LEAVES -> unchanged (2 sites)

Every skip-safe site already did try: json.loads(line) except ValueError: continue, so
skip is not a new posture there -- it is the posture the site already had, now reachable
for length as well as for syntax.

One site moved from skip to abort during review, and the reason is worth recording.
The first revision classified subagent_cost.py skip-safe by tracing _read_samples to
read_learned_cost, a read-only p90 aggregation. It has a SECOND consumer,
compact_cost_log, described above. GPT 5.6 review caught it. The lesson is that "does
this feed a rewrite" has to be answered by enumerating ALL consumers of the reader, not
the first one; a single-consumer trace is how a skip/abort call goes wrong.

Why the notification merge is NOT in this PR

It was converted here, and then backed out on the conductor's ruling after four
successive review rounds each found one more defect in it. That history is the argument,
so it is on the record rather than summarised away:

round	property the copy must preserve	how it failed
4	record boundary	a bare carriage return split a record, so the merge reported completion having merged nothing
5	dedupe-key type	json.loads(raw).get("ts") raised AttributeError on non-object JSON
6	dedupe-key hashability	a list or dict ts raised TypeError on set insert
6	framing	an unterminated record on either side glued two records into one corrupt line
7	encoding	an invalid-UTF-8 record poisons the whole destination file

Every other site in this audit is a CONSUMER: it reads, counts, displays, or decides,
and "hand back exactly what is on disk, or refuse" is a complete contract that the
reader alone can satisfy. The merge is a PRODUCER -- what it reads becomes the content
of a second durable file -- so the bytes must also be valid FOR THE DESTINATION. That is
a second contract, and nobody wrote it down, which is why it was being discovered one
review round at a time.

Four rounds is one missing specification found four times, not four separate mistakes,
and patching a fifth property would have traded another review round per undiscovered
invariant. So the merge is filed as the bug it actually is -- an invalid-UTF-8 record
empties live notification history -- with the write-side contract spelled out up front:
#7771. snapshot.py is unchanged from main here, so this is not a regression, it is the
pre-existing state.

The audit scanner lists it in _DEFERRED_READERS with that reasoning, so the deferral is
recorded in an executable place rather than only in this description.

Two smaller things the conversions forced

backfill.py's no-follow open is split into a shared _no_follow_fd guard with text and
binary wrappers, so the O_NOFOLLOW rationale is stated once and cannot drift.

An at-cap record ending CRLF is now accepted rather than refused by one byte. This
reverses a pin PR #7297 made deliberately, and the maintainer should see it as a
choice rather than a cleanup. That pin's stated cost was that buying the byte back
"would cost the reader its single invariant (a return shorter than cap+1 is a whole
record)" -- true of the old reader, which decided over-cap straight from one
readline(cap + 1) length. The shared reader has no such invariant left to lose: it
already defers a trailing carriage return across reads, because it must not split a
CRLF whose line feed has not arrived. So the byte is free, and the record is legal by
the cap's own definition -- its body IS cap bytes. Left refused, it suppressed a real
participation entry in members.read_activity. One commit to revert if you disagree.

What the abort site does instead of skipping

read_activity keeps its public signature and its documented non-raising contract -- 25
existing tests depend on it -- and becomes a thin wrapper over a new
_read_activity_checked that also returns whether the read was complete. Only the one
caller that WRITES honours the flag, and it fails closed by declining to append. That is
the same False the function's existing blanket handler already returns for any other
read failure, so no caller learns a new failure mode. subagent_cost uses the same
shape: the percentile consumer keeps degrading, and compact_cost_log declines to
compact on an incomplete read.

Tests

161 tests pass across the four affected files, 40 of them added by this PR
(test_jsonl_util.py +29 including a 3-test scanner class, test_members.py +6,
test_subagent_cost.py +4, test_session_digest.py +1 rewritten).

Two are behaviourally red against pristine base source, verified by copying the new test
files into a clean origin/main worktree and running them there (the cap fixtures use
raising=False, the same idiom as test_session_digest's create=True, so the files
RUN on base and fail on behaviour rather than erroring on a missing name):

• test_over_cap_record_refuses_to_append -- base skips the junk line, finds no match,
  and appends. AssertionError: must not append when the log could not be read in full.
• test_compaction_refuses_to_run_on_an_incomplete_read -- base rewrites the log from a
  partial read. AssertionError: compaction rewrote the log from a partial read, permanently deleting the record the reader refused. This is the regression test for
  GPT's finding.

test_every_handle_iteration_is_bounded_or_excused reports 0 unexcused sites on this
branch. It cannot be run against base source to produce a matching count, because the
test module's imports (RECORD_CAP and the reader names) do not exist there and
collection fails -- so that comparison was done by running the scanner logic standalone,
and no base number is claimed from the test itself.

That scanner is the piece I would keep if I could keep only one: it makes the audit
executable instead of a claim in this description. It scans the package for the shape and
requires every instance to be converted, excused with a reason, or listed as a tracked
deferral, so a new unbounded reader cannot land unnoticed and a converted one cannot
regress. A companion test guards the guard -- it asserts the detector still finds the
listed sites, so a scanner that silently stopped matching anything would fail rather than
pass. Detection is by the handle-name vocabulary rather than a fixed window after the
with, because a distance-based match missed sel.py:2522, where the handle is bound
several lines earlier.

The scanner's claim is deliberately no broader than the shape it detects. A whole-file
read spelled path.read_text().splitlines() has the same harm and is invisible to it;
learn.py, the auto_improvement ledger, and the meetings store each hold one, and
learn.py's feeds a _write_all rewrite, which is the abort-required pattern. Those are
a different reader shape than #6345 enumerates and are left for a follow-up rather than
swept in here.

Refactoring session_digest onto the shared reader is covered by its existing tests,
which pass unchanged -- including the spy test asserting every read carries a limit and
the handle is never iterated. That is the evidence the promotion is behaviour-preserving
rather than a rewrite.

Mutation-verified on the current reader core, 4/4 killed: readline(cap) instead of
cap + 1, the boundary terminator check dropped, the drain loop removed, and the strict
variant made to drain its hostile tail. The performance guard is mutation-checked too --
re-slicing the buffer per record makes
test_many_records_in_one_read_scale_linearly fail with 6.722s for 200000 records
against its 1.0s ceiling.

That test's threshold is measured, not guessed, because the first version of it was
WRONG: it compared 4,000 records against 8,000 with a 6x growth bound and passed the
quadratic mutant, since files that small never let the quadratic term show. Re-derived at
200,000 records: 0.10s for the shipped reader, 1.84s with a separate find per
terminator, 8.11s re-slicing per record. An absolute ceiling rather than a growth ratio,
because at these absolute times runner noise can move a ratio.

flake8, isort and mypy are clean on all 14 files. The repo's own gates pass with
real diff scope: check_black_formatting.py, check_sync_io_in_async.py. stub.py is
unformatted under black, but it is in .github/black-baseline.txt and the complaint is
pre-existing argparse help-string indentation this diff never touches, so it is left
alone rather than graduated out of the baseline.

Manual verification

The cap's floor is the one input unit tests cannot supply, and it is inherited rather
than re-derived: PR #7297 measured it with wc -L across both transcript trees on a live
install (30,351 kiro-cli logs, 27 GB) to find the longest record of any kind. Those are
the numbers quoted above. This PR adds a test asserting the constant still clears both
that measurement and the base64-expanded MAX_IMAGE_BYTES_PER_MESSAGE ceiling, so the
derivation is checked rather than trusted to a comment.

Everything else is asserted directly: the allocation bound by the spy-handle test, the
boundary by the at-cap / one-over / unterminated-at-cap tests, the byte-versus-code-point
distinction by a 240-byte 60-character record refused by a 200-byte cap, and the drain by
a file that is one line whose tail forges a complete record. A run against a real
multi-gigabyte planted file would add no evidence those do not already give.

Related Issues

Refs #6345 -- the audit it asks for is complete, but one enumerated site is deferred to
#7771 rather than converted, so this does not close it.
Refs #7771 (the deferred notification merge, filed as a data-loss bug)
Refs #6312, #7297 (the two earlier instances this generalises from)

Pattern harvest

Rule candidate: review-prompt

Pattern: for line in <handle> over a tree any agent can write is an unbounded
allocation -- one record's length is attacker-controlled. The reader must pass a limit to
readline, and then the per-call-site question is what an over-cap record does: SKIP
where the read is read-only and degradable, ABORT where its output feeds a rewrite or any
other durable decision. Four corollaries learned here. The limit must be in BYTES, which
means reading binary, because a character limit is not a memory bound. A file-size
rotation cap does not bound one record, because rotation only fires on the next append.
When a fix claims to close a defect CLASS, the enumeration itself is the risky step: a
grep keyed on the loop VARIABLE name misses instances that spell it differently, so the
audit belongs in a test that fails when a new instance appears, not in a table in a pull
request description. And a reader whose output is WRITTEN BACK to a durable file is a
different problem from one that is merely consumed -- it needs a stated write-side
contract (boundary, key type, framing, encoding) before any code, because otherwise each
property gets discovered one review round at a time.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 2e1e80e1120a352785059b73d8f7c79eb1007f59 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix at the right layer: one shared bounded reader with an explicit per-site skip/abort judgement, made regression-proof by an executable audit.

Suggestions

• The acknowledged read_text().splitlines() remainder — especially learn.py, which feeds a _write_all rewrite and so carries the exact permanent-deletion harm compact_cost_log was fixed for — is deferred with no issue number, unlike the snapshot merge (snapshot notification merge: an invalid-UTF-8 record silently empties live notification history #7771); file it before merge so the abort-required case doesn't evaporate.

[DESIGN-REVIEWED] 2e1e80e

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 2e1e80e1120a352785059b73d8f7c79eb1007f59 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All counts verified. The change is a derived fix (issue #6345, prior PRs #6312/#7297) with two declared deferrals; the greps confirm strict_raw_records has zero consumers outside its own module and tests, no code discriminates the two exception subclasses, and the "whole-file reader" sibling class the scanner comment defers is much larger than the three sites it names.

First-Principles-Verdict: CONCERNS

strict_raw_records ships public for a caller deferred to #7771, and the deferral note undercounts the identical whole-file-reader root cause by an order of magnitude.

What this change ships

Intent: stop one crafted newline-free line in an agent-writable log from OOMing the gateway — a FIX.

1. Usage, telemetry, history and stub panels skip a hostile oversized record instead of allocating file-size memory — justified
2. Member dedupe declines to record when its log cannot be read in full — justified
3. Cost-log compaction refuses to rewrite the file from a partial read — justified
4. One shared bounded reader replaces session_digest's private copy — justified (deletes a duplicate)
5. An at-cap CRLF record is now read instead of refused by one byte — declared reversal, justified
6. A bare carriage return now ends a record where it previously didn't — declared, justified
7. Repo-wide test freezes the audit behind an excused-reader list — rides along, mechanism-level
8. strict_raw_records exported with no caller until deferred snapshot notification merge: an invalid-UTF-8 record silently empties live notification history #7771 — zero consumers
9. Snapshot notification merge left unbounded, deferred to snapshot notification merge: an invalid-UTF-8 record silently empties live notification history #7771 — declared deferral
10. Backfill opens transcripts binary behind the existing symlink guard — justified

Watch

• The excused-list comment defers "learn.py, the auto_improvement ledger/archive, and the meetings store." Grepping read_text(...).splitlines( under src/kiro_crew counts 40+ sites, many over the same agent-writable trees the PR's premise covers: cron_history.py ×7, session_pid.py ×10, memory.py:128, dashboard/state.py:7685, learn.py:328. Accepted-and-deferred is fine; the follow-up's scope note understates its tail.
• OversizedRecord and UndecodableRecord are discriminated nowhere in src/ (grep except Oversized|except Undecodable: 0 hits — every caller catches UnreadableRecord); only tests tell them apart.

Subtractions

• Rename strict_raw_records to _strict_raw_records (or fold its loop into strict_records) until snapshot notification merge: an invalid-UTF-8 record silently empties live notification history #7771 lands: 0 consumers outside jsonl_util.py and tests, and its docstring already admits the intended caller is the deferred merge.

[FIRST-PRINCIPLES-REVIEWED] 2e1e80e

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2e1e80e1120a352785059b73d8f7c79eb1007f59 — this comment is updated in place on each push.

Review details

I traced the framing state machine (_frames, _boundary_end, _body_len), the drop/pending-\r interplay, the read-bound readline(max(2, cap+2-len(buf))), and the caller conversions (skip vs. strict posture, OSError propagation in history_search, fail-closed flags in members/subagent_cost). Every candidate the discovery pass could have raised is either impossible in the framing-only design or covered by a compensating guard, and the two behavior shifts (text→universal-newline splitting, locale→explicit UTF-8) are unreachable-change or strictly-better. I could not ground a defect on any added or changed line.

No findings.

[OPUS-REVIEWED] 2e1e80e

Verdict parsed from the review's SHA-scoped output markers for commit 2e1e80e1120a352785059b73d8f7c79eb1007f59.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 2e1e80e1120a352785059b73d8f7c79eb1007f59: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 2e1e80e1120a352785059b73d8f7c79eb1007f59 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/members.py:529 -- invalid UTF-8 triggers UnreadableRecord, but "over-cap record" misdiagnoses it -> Fix: say “unreadable record.”
[GPT-REVIEWED] 2e1e80e

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 2e1e80e1120a352785059b73d8f7c79eb1007f59: <one-sentence reason>

[chenmingwei23]

Round 1 dispositions, all four findings ACCEPTED and fixed in a97046964.

BLOCKING -- subagent_cost.py skip lets compaction erase valid records: real, and the most useful finding on this PR. Verified: compact_cost_log calls _read_samples() and then os.replace(tmp, path), so it rewrites the log from what the reader parsed. A skipped over-cap record is therefore deleted permanently on the next compaction. My audit had classified this site skip-safe by tracing the reader to read_learned_cost (read-only p90) and stopping at the first consumer -- it missed the second one.

I did not take the suggested remedy of reverting the conversion, because that leaves the unbounded allocation in place; reverting trades a data-loss bug for the OOM bug the PR exists to close. Instead the site now uses the same checked-reader shape already used for members.py: _read_samples_checked() returns the rows plus a completeness flag, _read_samples() stays the degrading view for the percentile consumer, and compact_cost_log fails closed -- it logs and declines to compact rather than trimming from a partial read. Leaving the log untrimmed costs bounded disk; compacting would cost data. That reaches the fix's stated end ("oversized records abort compaction") without reopening the hole.

Regression test test_compaction_refuses_to_run_on_an_incomplete_read is red on pristine origin/main with AssertionError: compaction rewrote the log from a partial read, permanently deleting the record the reader refused.

FINDING -- jsonl_util.py byte-bound claim: real, and I had inherited the error. "capping the read at N bytes caps the decoded str at N bytes too" is false under errors="replace": each undecodable byte becomes a U+FFFD costing 2 or 4 bytes of str depending on the record's widest code point, so 200 bytes of 0xff decode to a 400-byte str. The comment now says the decoded str is bounded by a small constant multiple of N, not by N, and states why -- which is still a bound flat in FILE size, the property the cap exists for. Note this wording came verbatim from the already-merged #7297, so the same sentence is wrong on main; it is corrected here because this diff moves the line.

FINDING -- OversizedRecord "raised only by strict_records": real. strict_raw_records raises it too. Both are now named.

FINDING -- members.py "contributes nothing": real contradiction. The generation contributes the prefix it read before the over-cap record. Docstring now says so, and explains why the flag matters: a prefix is indistinguishable from the whole log without it.

FINDING -- snapshot.py "left exactly as it was found": real overstatement. True only for the destination-scan abort, which happens before the append handle opens. A source-side abort keeps the records already appended. The docstring now separates the two cases and notes the retry is idempotent because existing is rebuilt from the now-larger live file.

Also rebased onto 2ba6fb53e (main advanced by one commit while this was in review).

[chenmingwei23]

Round 2 dispositions: both BLOCKING findings ACCEPTED and fixed in 01b6b08e2.

They turned out to share one root cause, so they are fixed at the chokepoint rather than patched separately. The strict readers had inherited their decode and boundary semantics from the skip reader, and those are only safe for a caller that counts or displays. An abort caller PERSISTS what it read, so any silent transformation of a record is written back:

• bare carriage return (jsonl_util.py:227): real. These files were read in TEXT mode, where universal-newline translation made a lone \r a record terminator. Binary reads split only on \n, so two carriage-return-delimited records arrive glued into one unparseable line. For a skip caller that costs a count -- its documented posture. For record_activity the prior entry disappears and the probe appends a duplicate. My docstring also mis-stated the consequence ("read as a single over-cap record and skipped"): two small glued records are UNDER the cap, so they are yielded and then fail to parse. Corrected.
• lossy decode (jsonl_util.py:245): real. strict_records used errors="replace", and compact_cost_log then os.replaces the log with what it parsed, so U+FFFD is permanently substituted for the original bytes. On main this path raises UnicodeDecodeError instead (it is not caught by the reader's except (FileNotFoundError, OSError)), so the first revision of this PR turned a loud crash into silent corruption. That is strictly worse and the finding is correct to block it.

The fix gives the strict readers one guarantee -- a record they yield is exactly the record on disk -- and makes every other outcome stop the read:

• UnreadableRecord base class, with OversizedRecord, UndecodableRecord and AmbiguousRecordBoundary under it. Callers catch the base, so a future refusal reason cannot leak past a caller's except (pinned by test_every_strict_refusal_is_one_catchable_class).
• strict_records decodes with errors="strict" and raises UndecodableRecord.
• Both strict readers refuse a record whose body contains a bare carriage return. A trailing \r\n still passes -- that carriage return rides on the terminator. No writer of these files emits a bare carriage return (json.dumps escapes one inside a string as two ASCII characters), so this is free on real data and fail-closed on a planted file.
• The skip readers are deliberately unchanged: replacement decoding and the carriage-return narrowing are correct for a caller whose worst case is one missing count. The module docstring now states that the two postures differ in more than the over-cap branch, and why.

Verification. Six new tests. Two are site-level and cover exactly the consequences named above:

• test_compaction_refuses_to_run_on_an_undecodable_record -- red on pristine origin/main with UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 50, green here, log bytes unchanged.
• test_a_cr_delimited_log_does_not_produce_a_duplicate -- this one is honest about what it guards: it PASSES on main (text mode parses the carriage return correctly) and is red against this PR's own previous revision a97046964 with assert True is False, i.e. it reproduces the duplicate append this finding predicted and now prevents. It guards the binary conversion, not a defect on main.

235 targeted tests pass; flake8, isort, black and mypy clean on the changed files; check_black_formatting.py and check_sync_io_in_async.py pass with real diff scope.

One related thing left deliberately unchanged, stated so it is not mistaken for an oversight: _merge_notifications still DROPS a record it cannot parse, as it did before. Copying it through would be worse rather than better -- the dedupe key comes from ts, so an unparseable record has no stable identity and would be re-appended on every subsequent merge instead of deduplicated. Its bytes are never transformed, which is the property this round is about.

[chenmingwei23]

Round 3: all five reviewer lanes are non-blocking on 01b6b08e2. Two advisories remain; one is FIXED in edce829ed, one is DECLINED with reasoning and a disclosure the finding did not reach.

First Principles Watch -- the scanner claims more than it detects: ACCEPTED, fixed. Correct, and it was my overstatement. The comment said a new unbounded reader "cannot land unnoticed" when the scanner only matches for <var> in <handle>:. The comment and the class docstring now scope the claim to that shape and name the gap explicitly, including that learn.py's whole-file reader feeds a _write_all rewrite -- the read-feeds-rewrite pattern this PR classifies abort-required. Those five whole-file readers are a different reader shape than #6345 enumerates, so they stay out and are named for a follow-up rather than swept in. Thank you for grepping the adjacent spelling; that is exactly the failure mode this PR's own pattern-harvest note warns about.

GPT -- telemetry.py replaces invalid UTF-8 instead of rejecting the shard: real, DECLINED, and broader than reported. I checked each converted site's pre-existing decode contract rather than just the one named, and the honest picture is worse than the finding says:

• Already lossy before this PR, so unchanged: session_digest (errors="replace" since well before fix(session-digest): bound the record reads over agent-writable session trees #7297), backfill (_open_no_follow uses errors="replace"), stub.py (errors="replace").
• STRICT before this PR, so genuinely changed by me: telemetry.py (1), usage.py (7), history_search.py (1) -- nine sites, not one.

At those nine, main decodes strictly and its except (OSError, UnicodeDecodeError): continue then discards the WHOLE FILE. So the delta is: main loses every record in a shard for one bad byte; this PR keeps the shard and may aggregate one record whose string field holds a U+FFFD. I am declining the fix because that trade goes the right way for these call sites, and the reason is not "it is only advisory":

1. Every one of the nine is read-only aggregation for a panel. Nothing is persisted, so a replaced character cannot propagate the way it would through compact_cost_log -- which is why THAT site got strict_records and these did not.
2. The values these panels sum are numbers, which are ASCII; a replacement character can only land in a label or name. The worst outcome is one mis-bucketed row, against main's outcome of an empty panel.
3. Per-record degradation is strictly more faithful than per-file. Reverting to strict would restore a behaviour where one planted byte hides a whole day of usage data.

The alternative I considered and did not take: a fifth reader (skip posture, strict decode, skip the record it cannot decode) routed at those nine sites. That is better than both main and this PR -- it never aggregates altered JSON AND keeps the rest of the shard -- but it adds public API and nine call-site changes to a diff that has just gone green on all five lanes, for a display artefact with no durable consequence. I would rather ship it as a follow-up than grow this PR again; say the word and I will fold it in instead.

Recorded locally as a backlog item either way, together with the five whole-file readers above.

[chenmingwei23]

Round 4: the BLOCKING finding is ACCEPTED and fixed in 41ed302e5.

snapshot.py:2190 -- a restore reports completion having merged nothing: real. A bare carriage return in notifications.jsonl raised AmbiguousRecordBoundary, _merge_notifications caught it and returned, and the restore carried on reporting success. Confirmed by reading the chain rather than the summary.

I want to be explicit that this is NOT the same finding twice, because it touches the same theme as round 2. Round 2 offered two remedies -- "preserve bare-carriage-return boundaries OR fail closed when a chunk contains one". I took fail-closed. This round shows what that choice costs at a site whose caller treats the abort as a no-op and continues. That is legitimate narrowing after seeing the consequence, not a demand to undo a remedy the same lane previously required, so it gets fixed rather than escalated.

Fix: preserve the boundaries in the shared core, which is the option I should have taken first. _raw_records now splits on the full universal-newline set (\n, \r\n, bare \r) through a new _boundary_end helper, so a binary read lands records exactly where the text-mode read it replaced did. The boundary set is closed and small, so this is bounded work rather than the open-ended grammar-matching that would deserve a refusal.

Three properties worth naming, each with a test:

• The cap is judged PER RECORD, after splitting. One read is at most cap + 1 bytes and may hold several small carriage-return-delimited records; the previous pre-split check refused all of them together. The check stays nearly free because any record that ENDS inside a read has a body of at most cap, so only an unterminated tail can exceed it -- and that tail is measured every read, so an over-cap record is still caught without waiting for a terminator that may never arrive.
• A trailing carriage return is deliberately not a boundary until more bytes arrive. It may be the first half of a \r\n split across two reads; treating it as a terminator there would invent a record end and leave the following \n looking like an empty record. At EOF the remainder is yielded whole, so a file genuinely ending in a bare carriage return still produces exactly one final record.
• Peak memory is now roughly TWICE the cap (one unterminated tail plus one read), not one. The docstrings and the RECORD_CAP comment claimed ~1x; both corrected rather than left as a quiet overstatement.

The change SUBTRACTS surface: AmbiguousRecordBoundary and the _refuse_bare_cr helper are deleted, the exception hierarchy drops from three subclasses to two, and the "documented narrowing" caveat comes out of two reader docstrings and the module docstring because it stops being true. If a fix for a boundary finding had grown the surface instead, that would have been the signal to stop and reconsider.

Verification. 236 targeted tests pass, including the 32 pre-existing session_digest tests -- unchanged, which is what shows the core rewrite is behaviour-preserving on the path that was already reviewed. Five new tests cover the bare-carriage-return boundary in both postures, CRLF as one terminator, a CRLF straddling two reads, a file ending in a bare carriage return, and the per-record cap. Mutation-verified 4/4 killed: bare carriage return never a boundary (3 failures), trailing carriage return split immediately (1), CRLF treated as two records (2), over-cap tail not drained (7).

test_a_cr_delimited_log_does_not_produce_a_duplicate in test_members.py was updated rather than deleted: its mechanism changes from "the record is refused" to "the record parses as two records", and it still asserts no duplicate is appended, plus that both records are now read.

flake8, isort, black and mypy clean on all changed files; check_black_formatting.py and check_sync_io_in_async.py pass with real diff scope.

Separately: the red Backend Tests (Windows) (3) is not from this PR. test_session_storage.py::TestSidecarFiles::test_an_unrecognised_sidecar_moves_with_its_session fails with "all 1 selected session(s) were resumed while being staged"; this diff touches no session_storage file, and that exact shard PASSED on the previous head edce829ed with byte-identical code from me -- the only delta was a rebase onto 50 new commits from main. Left alone rather than folded in; it needs a re-run once its workflow finishes.

[chenmingwei23]

Round 9 is fixed in cc745f350, but not with the one-line remedy -- by separating FRAMING from POLICY, so the class the last three rounds came from is unrepresentable rather than patched a fourth time.

Why the structure changed at round 9 rather than round 8

I argued last round that rounds 6, 7 and 8 were "one property approached three times" and that replacing the mechanism would end it. Round 9 falsified that, and it belongs on the record as falsified rather than replaced with a fresh argument for patching:

round	finding	direction of the error
6	quadratic copying -- and my own first fix was still quadratic, because the line-feed search walked to end-of-buffer per record	cost
7	held carriage return counted as body, so a legal at-cap record was refused	cap too STRICT
8	a carried tail completed mid-read bypassed the cap; up to ~2x cap could be yielded and decoded	cap too LOOSE
9	held carriage return erased when dropping, so the next record's terminator ended the drop and a valid record vanished	drop too GREEDY

Rounds 7, 8 and 9 are not three properties, and they are not one property either. They are three consequences of ONE design decision. A trailing carriage return must be HELD rather than split, because its line feed may be in the next read -- and the old reader made cap and drop decisions by reading that same raw buffer. Framing state and policy state were entangled in one loop, so every framing subtlety surfaced as a policy bug, in a different direction each time. A fourth patch would have been the fourth direction, not the last one.

What the separation is

_frames(handle, cap) now does framing and nothing else: it owns the buffer, the boundaries, and the cap as a memory bound, and it yields either a COMPLETE record or an _OVERSIZED marker. It decides nothing about what over-cap MEANS. The two policy layers consume that and never see a buffer -- bounded_raw_records counts the marker and continues, strict_raw_records raises on it.

None of rounds 7, 8 or 9 is expressible in that shape, because the only things crossing the boundary are a whole record and a marker. There is no buffer for policy to miscount, no carried tail for it to miss, and no buffer for it to clear.

Two things worth flagging in the design:

The drain switch is gone, and laziness replaced it. The old reader took drain=True/False so the strict path would not walk a multi-GB tail it was about to discard. That is now free: raising abandons the framing generator, and because generators are lazy the discard it would have done next simply never runs. Same guarantee, no flag to pass or get wrong.

_OVERSIZED is a distinct type, not None. A marker type cannot be confused with "no record" the way a sentinel None can, which matters because the policy layers now branch on exactly one thing.

Tests

All three findings are pinned so the new structure demonstrably preserves the old fixes rather than being assumed to:

• test_dropping_an_over_cap_record_does_not_eat_the_next_one -- round 9. Mutation-checked: restoring the erasure (buf = b"") fails it with the record after the dropped one was eaten: [b'\r'].
• test_a_carried_tail_completed_by_a_later_read_is_still_capped -- round 8, still green under the new structure.
• test_an_exactly_at_cap_record_ending_crlf_is_accepted -- round 7, still green.

Each layout is minimal and every part of it is load-bearing; the docstrings say which part does what, because these shapes are not obvious and a future edit that "simplifies" one silently stops testing anything. Round 9's needs a leading carriage return (so the first full read splits and a tail is carried), a tail of exactly cap (so it passes the unterminated-tail check), and a following full read ending on a held carriage return (so the buffer is over-cap AND ends on the terminator being erased).

210 tests pass across the affected files including test_security_posture.py. flake8, isort, mypy clean; black and sync-io gates pass with real diff scope.

Bare carriage return support is retained. I offered dropping it as the aggressive option and argued the contract for it is weaker than this description claims. That was not taken: the description's argument stands unless a maintainer overrules it, so universal-newline boundaries are unchanged and a record still begins and ends exactly where the text-mode iteration this replaces put it.

My line stands, and the span it applies to is now the new one: a blocking finding inside _frames and I stop and escalate rather than patching again.

[chenmingwei23]

Two reds on cc745f350, and they had a single common shape: the coverage-instrumented shard. Head is now f4d0e7704. Coverage Gate and PR Readiness were pure cascades -- the only failing step in the gate job was "Require upstream coverage jobs to have succeeded".

My perf test was the flake, and my earlier reasoning for it was backwards

test_many_records_in_one_read_scale_linearly failed with 1.101s for 200000 records suggests quadratic scanning against the 1.0s ceiling I set two rounds ago. Locally that reader does 200,000 records in 0.10s, so the assertion had what looked like a 10x margin.

ci.yml:692 is why it went: coverage is enabled for the 3.12 shards only (--cov=kiro_crew there, --no-cov everywhere else). The test passed every 3.10 shard and every Windows shard and failed exactly one job -- the traced one. Tracing charges per Python line executed, and the per-record bookkeeping in this reader is all Python, so 0.10s became 1.101s and ate the margin.

When I introduced that ceiling I explicitly rejected a growth ratio, on the grounds that "at these absolute times runner noise can move a ratio". That was backwards. An absolute ceiling encodes the speed of the machine that measured it, so it is the environment-sensitive choice; a ratio cancels the machine out, because both measurements pay the same constant factor. I had the robustness argument exactly inverted, and CI billed me for it.

Rewritten as a ratio, with each piece there for a stated reason:

• Ratio, not ceiling: 100,000 against 200,000 records. Measured on the shipped reader and on both quadratic forms it replaced -- linear 2.02x, one bytes.find per terminator 3.86x, re-slicing per record 6.9x. The threshold is 3.0x, between them.
• process_time, not perf_counter: excludes time this process was not scheduled, so a busy runner does not leak in.
• Best of three attempts per size: scheduling noise can only make a sample slower, so the minimum is the honest estimate.
• Skipped under a tracer, and this is the part that is not merely defensive. Tracing does not just slow things down uniformly, it breaks the ratio's premise: it charges per Python line while leaving the C-level scanning and copying -- which is where the quadratic cost actually lives -- untouched. So it inflates the linear term and flattens the very difference being measured. A ratio under coverage could mask a real regression, which is worse than not running.

Re-verified by mutation, because a perf test that cannot fail is decoration: re-slicing per record fails it at 6.63x, and the subtler separate-find form fails it at 3.95x. Skip path confirmed under a real tracer (coverage run -m pytest reports 1 skipped, not a failure).

The other failure is a test-isolation defect, and it is the second of its kind

test_dashboard_handlers_core_coverage.py::TestAgentSettingsPut::test_resent_unchanged_value_does_not_ask_for_a_restart failed with restart_required: True where False was expected.

What I can state with evidence: this diff touches neither dashboard/handlers/core.py nor anything in the agent-settings path. On pristine main the test passes alone, passes as a whole file (135 passed), and passes alongside 49 related dashboard/config/agent test files in one worker (1,946 passed). So it is not deterministic from its own file, and it is order- or state-dependent -- the same class as #7776, which I proved and filed earlier on this PR.

The mechanism is consistent with that reading: restart_required is computed as agent.get(key) != before.get(key), so getting True after re-sending an unchanged value means before was not the config the test had just seeded. That is pre-update state arriving from somewhere other than the fixture.

What I have NOT done is identify the polluting test, because reproducing it needs that shard's particular 20,000-test composition and I could not narrow it by inspection. Rather than keep guessing, the perf fix above produces a fresh full run over the same shard composition, which settles whether it is deterministic. If it recurs on f4d0e7704 I will bisect the shard properly and file it; if it does not, it is a second instance of the shared-state fragility #7776 already describes, and belongs there.

Not patching it here either way: it is not this diff's defect, and guessing at a fix for a failure I cannot reproduce would be worse than leaving it visible.

210 targeted tests pass. flake8, isort, mypy, black and sync-io gates all clean before push -- gated on an explicit flag this time, after I pushed a head earlier tonight whose lint had actually failed because my && guarded an echo instead of the push.

[chenmingwei23]

Round 10: the finding is REAL, I measured it, and I am NOT patching it -- it is inside _frames, which is the span I committed publicly to stopping on one round ago. Head is unchanged at f4d0e7704.

Measured, not relayed

I measured peak allocation with tracemalloc at a 4 MiB cap rather than accepting the arithmetic, and the numbers reframe the finding in a way the report does not:

reader	peak / cap
main's merged _bounded_lines (PR #7297)	3.03x
this branch (f4d0e7704)	4.03x
this branch with the suggested remedy	3.03x

Three things follow, and two of them contradict things I had believed:

1. It is a real regression, +1x cap over main. I first assumed the peak was inherited, since the shape looked the same. It is not: main reads ONE line per readline and keeps no carried buffer, while _frames must accumulate across reads -- because a \r\n can straddle a read boundary and a bare \r is a boundary. The extra 1x is the price of universal-newline framing, which is a cost I introduced and never accounted for.

2. My docstring's "roughly twice the cap" was never true -- not even of main. I wrote that claim and repeated it in the description. Measured, main is 3x and this branch is 4x. That is a documentation defect regardless of which option below is chosen.

3. The suggested remedy restores parity with main exactly, but it is not a one-liner. readline(max(2, cap + 2 - len(buf))) gets 4.03x back to 3.03x. It also fails two spy tests -- test_handle_is_never_read_without_a_limit here and test_transcript_handle_is_never_read_unbounded in test_session_digest.py, which is MAIN's -- because both assert limit <= cap + 1 and the first read becomes cap + 2.

That + 2 is not arbitrary and cannot be trimmed to + 1: a legal at-cap record with a CRLF terminator is cap + 2 bytes, so a buffer capped at cap + 1 could never assemble one, which is exactly the round-7 bug returning. So the remedy necessarily relaxes an asserted read bound, and it is coupled to round 7.

I also tested my own idea before recommending it -- switching the buffer to a bytearray so += extends in place instead of allocating the concatenation. Measured: still 4.03x, no improvement. The double-allocation is not the dominant term; the buffer plus the chunk is. Reporting that because it would otherwise look like the obvious fix.

Why I am escalating rather than applying it

jsonl_util.py:253 is inside _frames. One round ago, when the conductor approved separating framing from policy, I wrote that a blocking finding inside _frames would mean stopping rather than patching again -- specifically so that I could not talk myself into a fourth patch the way I did at round 8, where I argued the class was converging and round 9 proved it was not.

I will not decide this one either, but I will say plainly what I think, since the count is now the argument: this does NOT look like another direction of the framing-versus-policy entanglement that rounds 7, 8 and 9 were. That was about WHICH records get emitted. This is a memory constant factor in how much is read, it is present in every revision of this code including the first, and it has a bounded fix in one expression. On that reading it is a genuine new finding rather than the class recurring -- but I said I would stop, so the call is yours.

Options

A. Take the remedy and relax both spy assertions to <= cap + 2 (my recommendation). Restores parity with main at 3.03x. The spy tests' PURPOSE is "no read is unbounded", which survives; the exact constant was incidental. Document cap + 2 as forced by the at-cap CRLF case so it reads as a derived number rather than a magic one. Cost: edits a bound test_session_digest.py asserts, which is main's.

B. Correct the documented bound to the measured ~4x and change no code. Zero risk, and it fixes the defect this PR actually introduced -- the false claim. Cost: keeps the +1x regression over main, roughly 516 MB transient at the 128 MiB cap.

C. Get to a true 2x. Neither main nor this branch has ever been 2x, so this is new work, not a restoration -- most likely readinto against a preallocated buffer. Right if the number matters on constrained hosts; the largest change, and in the read-sizing area that produced round 8.

Not an option, so nobody spends time on it: lowering RECORD_CAP cannot help. It must clear the base64-expanded MAX_IMAGE_BYTES_PER_MESSAGE ceiling of ~85 MB and the largest measured live record of 77,920,032 bytes, and 128 MiB is already the smallest round value that does. Halving it would truncate real sessions.

Whatever you choose, the docstring and the pull request description both need the measured number, and I will fold in the non-blocking wording finding at members.py:518 and subagent_cost.py:108 at the same time -- "over-cap record" understates a handler that also catches invalid UTF-8. That one has now been raised twice and is two lines.

Everything else on this head: 40 checks pass, no other failures, no unresolved review threads.

[chenmingwei23]

Round 10 is fixed in 2e1e80e11. Peak allocation is back to 3.03x the cap, exact parity with main, re-measured after the change rather than predicted.

The fix. Each read is now bounded by what the carried record has left -- readline(max(2, cap + 2 - len(buf))) -- instead of asking for a full cap regardless of what the tail already held. That was the whole of the +1x: a nearly-full buffer plus a full-size read.

Both spy assertions are relaxed to cap + 2, and the derivation is in the comment so the number cannot later read as arbitrary. A legal at-cap record ending in a CRLF is cap + 2 bytes, so a buffer that could only ever hold cap + 1 could not assemble one and would refuse it -- the round-7 bug returning. What those tests exist to pin is that no read is UNBOUNDED, which is unchanged. One of the two is test_session_digest.py's, which is main's, so it is called out rather than quietly edited.

Three false claims removed. The _frames docstring, the RECORD_CAP comment, and the pull request description all said the peak was "roughly twice the cap". It was never true -- not of this reader and not of main's at 3.03x. All three now carry the measured numbers and the table. The two disk-side "twice the cap" statements about rotation are correct and untouched; those are about total disk from keeping one generation, not resident memory.

A verification result worth reporting, because it changes what one test is worth. After the fix I re-ran the mutation check on the per-piece body cap, and test_a_carried_tail_completed_by_a_later_read_is_still_capped -- the round-8 regression test -- stayed green with that check disabled. It no longer reaches the branch it was written for, and it cannot: bounding reads by the record's remainder means the buffer can never grow to the size that scenario needed, so the input is now refused earlier, by the unterminated-tail check.

The guard itself is not lost -- disabling the per-piece check still fails test_record_one_byte_over_cap_is_skipped here and test_record_one_over_cap_is_skipped in test_session_digest.py, which is where that branch is actually pinned. But the test's docstring claimed a mechanism that is no longer the active one, which is a trap for whoever edits this next, so it now states plainly which check refuses that input today, which test pins the per-piece branch, and that the distinction was established by mutation. It is kept because it still pins the OUTCOME for a shape that once escaped, and it would catch a future change that relaxed the read bound back.

I would rather report that than let a test sit there implying coverage it no longer provides.

Also folded in, since it had been raised twice and the ruling asked for it: members.py and subagent_cost.py no longer describe an UnreadableRecord as an "over-cap record". That exception also covers invalid UTF-8, and the subagent_cost one is an operator-facing warning, so naming only the cap pointed at a size problem that may not exist.

210 targeted tests pass. flake8, isort, mypy, black and sync-io all clean before the push, gated on an explicit flag.

[bolichen97]

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #8184 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8184: KEEP. Merged prerequisite, not coverage. The reader and exception types 8184 needs are on main; the notification merge itself is untouched there. Files: src/kiro_crew/jsonl_util.py, test/test_jsonl_util.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.