Code review · P3 Task 3, the spool reader

review-code on autopilot/P3-t03, base a8d0c5b, 2026-10-30. The step is P1 Task 3. It adds:

• spectio.runner.spool.read_spool, which reads one launch's spool past a cursor, with caps and confinement;
• spectio.runner.types: SpoolBatch, REFUSED and EXCESS;
• in spectio_sdk.events: MAX_SPOOL_FILES, MAX_EVENT_NODES, MAX_EVENT_DEPTH and the file-name grammar SPOOL_FILE_NAME;
• a CI step in the product job that checks the import contracts in a fresh environment (SEC-P3T1-06);
• packages/spectio/tests/test_spool_reader.py, 61 tests.

The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. What the reader does is owned by api §14.1.

Round	Head	code-reviewer	security-reviewer
1	5f7f55d (the step: reader and CI step)	changes (0 blockers, 1 major, 5 minors)	changes (0 blockers, 2 majors, 5 minors)
2	cd78f18 (the round-1 fixes)	approve (0 blockers, 0 majors, 6 minors)	approve (0 blockers, 0 majors, 3 minors)
3	8058fb0 (the round-2 fixes)	approve (0 blockers, 0 majors, 2 minors)	approve (0 blockers, 0 majors, 1 minor)
after 3	a7a4610 (the round-3 fixes)	not re-reviewed; the orchestrator read the diff and re-ran the tests (152 passed)	not re-reviewed; same check

Rounds. Round 1 asked for changes (two security majors); round 2 approved, with minors. Round 3 was an extra review of the worker's fixes for those minors, made after approval (8058fb0); it approved, and its three minors were fixed in a7a4610, which the orchestrator checked without a further review. Round 3 is outside the two-round rule of review-code step 4: the exception of inbox #75 covers only rounds while a security blocker or major is open, and none was. Recorded in inbox #76.

Test-first. The worker's record: the first run failed at collection (ModuleNotFoundError, no reader yet); the round-1 tests failed 19 before the fix, the round-2 tests 17, the round-3 tests 3; all pass after each fix. Test command, passing at a7a4610 (61 tests in this file, 152 in the workspace):

uv run pytest packages/spectio/tests/test_spool_reader.py -v

CODE  step=P3-t03  review=approve b/M/m=0/1/13  security=approve b/M/m=0/2/9  rounds=3  simplify=changed -2/+1  tests=pass

The counts are per reviewer over the three rounds: code 1 major (CR-P3T3-01) and 13 minors (CR-02 to CR-14); security 2 majors (SEC-P3T3-01, -02) and 9 minors (SEC-03 to SEC-11). code-simplifier ran after round 3 (commit bd206f6, −2/+1): it removed the unreachable ENOTDIR refusal (CR-P3T3-12). The tests pass after it.

Measured by security-reviewer at the end: the worst read took 0.86 s with a traced peak of 11.1 MiB. On the round-1 head, 500 lines took 788 MiB.

Findings

The finding texts are summarised from the fix commits' messages and the round summaries. A pair such as CR-P3T3-01 / SEC-P3T3-03 is one problem found by both reviewers.

Id	Severity	Finding	Status
SEC-P3T3-01	major	Memory and time of one read were unbounded: only lines were capped, not the bytes of a read or the size of an event's data; on the round-1 head, 500 lines took 788 MiB.	fixed in 35fb19e: a read takes at most max_bytes (MAX_READ_BYTES, 2 MiB) from the files, skipped bytes included; data may hold at most MAX_EVENT_NODES (4096) values and MAX_EVENT_DEPTH (32) levels (test_req6_read_stops_at_max_bytes, test_req6_memory_bounded_on_nested_lines, test_req6_event_node_cap, test_req6_event_depth_cap)
SEC-P3T3-02	major	No owner or mode check: .spectio, events or a spool file of another user, or one writable by others, was read.	fixed in 35fb19e: .spectio and events must be private directories of the user (fstat after the O_NOFOLLOW open), else PermissionError; a file of another owner, or writable by group or others, is refused (foreign_owner, writable_by_others; the test_req12_* tests)
CR-P3T3-01 / SEC-P3T3-03	major / minor	An OSError on one file stopped the whole read.	fixed in 35fb19e: ELOOP, EACCES and ENXIO refuse the file (symlink, unreadable); any other error is counted read_error and the file is retried (test_unreadable_file_refused_and_the_next_one_read, test_read_error_retried_next_read)
CR-P3T3-02 / SEC-P3T3-04	minor	The directory listing was unbounded.	fixed in 35fb19e: at most MAX_DIR_ENTRIES (16 384) entries per read (test_directory_listing_cap)
CR-P3T3-03 / SEC-P3T3-05	minor	Junk names could take the cursor's file slots and grow the cursor.	fixed in 35fb19e: slots go to this launch's spool files only; a name outside the grammar is kept as a short hash, MAX_BAD_NAMES (64) at most; the rest is counted through a high-water mark, #excess (test_junk_names_leave_the_slots_to_spool_files, test_spool_file_cap)
CR-P3T3-04 / SEC-P3T3-06	minor	The fresh-environment import check resolved its dependencies anew, unlocked.	fixed in cd78f18: each environment installs uv export --frozen --no-dev --no-emit-workspace --package <pkg> with --require-hashes, then the workspace packages with --no-deps
CR-P3T3-05 / SEC-P3T3-07	minor	api §14.1 did not state the reader's rules: the caps, the grammar, the drop reasons, the cursor.	fixed by the docs of this step: api §14.1, "Dropped and counted, never executed", and its bounds table
CR-P3T3-06	minor	A file of exactly MAX_FILE_BYTES that grew was never counted file_too_large.	fixed in 35fb19e (test_req6_file_of_exactly_max_bytes_counted_when_it_grows)
CR-P3T3-07	minor	The files read came from the listing, so a listing cut at MAX_DIR_ENTRIES could skip a file admitted earlier.	fixed in 8058fb0: the files read come from the cursor, in name order (test_admitted_files_read_past_the_listing_cap)
CR-P3T3-08	minor	Unended lines in earlier files can use up the byte budget of a read and starve later files. It needs a buggy or hostile writer.	open, follow-up (Progress page)
CR-P3T3-09 / SEC-P3T3-10	minor	An event over the node or depth cap was counted invalid, and was fully parsed before the check.	fixed in 8058fb0: it is counted too_complex; a line with more than 2 × (4096 + 16) of [, { and , is dropped before it is parsed (test_too_complex_line_not_parsed)
CR-P3T3-10	minor	A file gone between the listing and the open was counted read_error.	fixed in 8058fb0: it is skipped (test_file_gone_between_listing_and_open)
CR-P3T3-11	minor	uv export ran without --quiet in the CI step.	fixed in 8058fb0: --quiet
CR-P3T3-12	minor	simplify: the ENOTDIR refusal is unreachable: a single name opened relative to an O_DIRECTORY descriptor cannot fail with it.	fixed by code-simplifier in bd206f6 (−2/+1)
SEC-P3T3-08	minor	Cursor values were not checked on entry.	fixed in 8058fb0: every value is checked on entry (an int, not a bool; REFUSED or 0 to MAX_FILE_BYTES for a file; REFUSED for a name hash; a count for #excess); anything else is reset and counted bad_cursor; too small max_lines or max_bytes raise ValueError (test_bad_cursor_values_refused, test_budgets_too_small_refused)
SEC-P3T3-09	minor	The reader trusts launch_dir and the path to it, and nothing named who checks them.	partly fixed in 8058fb0: the docstring names the callers, P1 Tasks 9 and 11. Remainder, follow-up: P1 Task 9 must create .spectio and .spectio/events at 0700 and walk the path per component as req. 10 says, else every read raises PermissionError
CR-P3T3-13 / SEC-P3T3-11	minor	A cursor key that is not a spool file name of this launch took a file slot, and another launch's file named in the cursor was opened.	fixed in a7a4610: such a key is dropped and counted bad_cursor once, and the file is never opened (test_foreign_cursor_key_dropped_once, test_other_launch_cursor_key_dropped_and_never_opened); verified by the orchestrator, not re-reviewed
CR-P3T3-14	minor	The worst-read timing test allowed 1 s, which a loaded CI runner could miss.	fixed in a7a4610: 5 s; test_too_complex_line_not_parsed proves the mechanism; verified by the orchestrator, not re-reviewed

Closed from Task 1's review (record): SEC-P3T1-04, the reader's rules, in 48ddf5b; SEC-P3T1-06, the fresh-environment import check, in 5f7f55d and cd78f18.