Code review · P3 Task 6, nf-spectio writes the spool

review-code on autopilot/P3-t06, base a758cf3, 2026-10-30. The step is P1 Task 6. It adds:

• engine-bridges/nf-spectio/src/spectio/SpectioPlugin.groovy: a TraceObserverV2 plugin, ported from the spike T2 prototype. The hooks enqueue allow-listed data; one writer thread appends envelopes to the spool and rotates;
• engine-bridges/nf-spectio/build.sh (builds the unpacked plugin without Gradle) and tests/run_local.sh (runs the synthetic pipeline with it);
• packages/spectio/tests/test_bridge_nextflow.py, 14 tests marked engines, skipped by the default pytest run (pyproject.toml);
• in CI: a step in pipelines-synthetic.yml that runs them on the Nextflow legs, and the bridge paths in ci.yml's PR filter.

The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. What the bridge writes is owned by api §14.1; how it fails, by architecture §13.3. The rules decided here are in inbox #82.

Round	Head or range	code-reviewer	security-reviewer
1	511976c (the step)	changes (0 blockers, 1 major, 7 minors)	approve (0 blockers, 0 majors, 5 minors)
2	694a42c (the round-1 fix)	approve (0 blockers, 0 majors, 5 minors)	approve (0 blockers, 0 majors, 5 minors)
3	2074d6d (fixes of three round-2 minors)	approve (0 blockers, 0 majors, 2 minors)	approve (0 blockers, 0 majors, 1 minor)

Minors include the simplify: finding, as in the earlier records.

Rounds. One fix round for the major, then both approved in round 2. The worker then fixed three round-2 minors (2074d6d), and round 3 reviewed that fix.

IDs repeat across rounds. Each round numbered its findings again, so each ID below carries its round: CR-P3T06-02 (r1) is not CR-P3T06-02 (r2). The commit messages use the bare IDs.

Test-first. The worker's record: each fix began with a failing engine test. Test command, passing at 17281b7 with Nextflow 26.10.4 (13 passed, 1 skipped as root):

uv run pytest -m engines packages/spectio/tests/test_bridge_nextflow.py -v

CODE  step=P3-t06  review=approve b/M/m=0/1/14  security=approve b/M/m=0/0/11  rounds=3  simplify=changed -1/+1  tests=pass

The counts are per reviewer over the three rounds, each finding counted in the round that raised it; a problem raised again counts again. code-simplifier ran after round 3 and made one change, 17281b7 (CR-P3T06-08 (r1)). Its baseline and its after-change run of the default gate were the same: pytest 214 passed, 14 deselected; ruff, mypy, lint-imports and uv lock --check clean. It ran the engine tests after the change only.

Findings

The finding texts are summarised from the round summaries and the fix commits' messages. A pair such as CR-P3T06-04 (r1) / SEC-P3T06-05 (r1) is one problem found by both reviewers.

Id	Severity	Finding	Status
CR-P3T06-01 (r1)	major	A write that keeps failing (block quota, ENOSPC, EFBIG) created a new spool file for each event. After 1024 files (the reader's MAX_SPOOL_FILES) every later file of the launch counts too_many_files, onFlowComplete included; at index 9999 the bridge stops writing for good.	fixed in 694a42c: a failed write does not rotate; the writer reopens only when the file is gone, and reuses an empty current file (test_writer_does_not_rotate_on_a_failed_write, under ulimit -f 16)
CR-P3T06-02 (r1)	minor	The close-time line "N events dropped: the queue was full" goes through the 100-line cap, so after 100 errors the drop count is lost, and nothing says lines were suppressed.	fixed in 2074d6d, as CR-P3T06-04 (r2)
CR-P3T06-03 (r1)	minor	Code comments cite Nextflow internals (Session.groovy:919-936, WorkflowMetadata.setErrorAttributes) with no tag or commit.	partly fixed: api §14.1 and architecture §13.3 now cite them at tag v26.10.4 (807ef3a), read 2026-10-30. Open: the code comments, follow-up FU-P3-10
CR-P3T06-04 (r1) / SEC-P3T06-05 (r1)	minor	The PR path filter runs the bridge test when the bridge or its test changes, not when the contract it replays through changes (spectio_sdk/events.py, spectio/runner/spool.py, the projector). A break shows first on main.	open, follow-up FU-P3-09
CR-P3T06-05 (r1)	minor	Plugin-Requires: >=25.04.0 declares a floor that CI never runs (the matrix is 26.10.4 and 25.10.9); architecture §13.4 says the range equals the versions tested in CI. The plan asks for the 25.04 floor; 25.04.8 was checked by hand only.	open, assumed: inbox #82 rule 4; the 25.04.8 CI leg is follow-up FU-P3-11; architecture §13.4 says so
CR-P3T06-06 (r1)	minor	With no framework jar, build.sh falls back to the launcher file as the classpath. That works only for a -dist launcher; otherwise java fails with an unrelated "could not find or load main class".	open, follow-up FU-P3-10
CR-P3T06-07 (r1)	minor	On a run with no failed task, errorMessage is Nextflow's own error (session.error.message), capped at 4096 characters, not filtered. A script or config error can quote source or paths.	open, assumed: inbox #82 rule 1; api §14.1 says so
CR-P3T06-08 (r1)	minor	simplify: the "no exit status yet" rule is written twice, in exit() and as a string comparison in trace().	applied by code-simplifier in 17281b7: trace() calls exit()
SEC-P3T06-01 (r1)	minor	The redaction reads WorkflowMetadata.errorMessage, whose content depends on the Nextflow version, and blanks it on one known condition only (session.fault.task != null). It fails open if a later Nextflow puts task output there under another condition; Plugin-Requires has no upper bound.	open, assumed: inbox #82 rules 1 and 4
SEC-P3T06-02 (r1)	minor	SEC-P3T1-09 / FU-P3-08 is closed in code but not in the spec: api §14.1 still says errorMessage is kept, and api §7 says the failure message is that errorMessage. Nextflow's own error text is still sent, up to 4096 characters.	spec fixed by this step's docs (api §14.1, api §7 runs.failure). Open: the residual, inbox #82 rule 1
SEC-P3T06-03 (r1)	minor	The writer does not apply the reader's confinement: createDirectories follows a symlinked .spectio; no owner or mode check of .spectio or events; files reopened by path after createFile; bridge.err appended whatever its owner or mode, and placed in <SPECTIO_SPOOL_DIR>/.. without checking that path. Matters in group-writable launch directories (terminal runs, architecture §13.8), not in the 0700 run directories of driven runs.	partly fixed in 2074d6d: every open passes NOFOLLOW_LINKS, and bridge.err must be a regular file. Open: P1 Task 25, the fd-relative writes of dev-env §15.3 req. 12
SEC-P3T06-04 (r1)	minor	The in-JVM queue is bounded by count (100 000 items), not by bytes: hundreds of MB of head heap while the writer stalls. The shutdown hook calls the synchronized writer.error / close after join(10 s), so a writer blocked in write() keeps the JVM from exiting.	hang fixed in 2074d6d, as SEC-P3T06-02 (r2). Open: the byte bound, inbox #82 rule 3
CR-P3T06-02 (r2)	minor	After the round-1 fix, a torn write followed by a rotation leaves an unended line that the reader never counts; the tracked size misses the partial bytes.	fixed in 2074d6d: the file is truncated back to size after a failed write (test_writer_truncates_a_failed_write)
CR-P3T06-03 (r2)	minor	Two new branches of the round-1 fix are untested (ending a torn line, reopening an empty file), and the file-count assertion (1 <= len(files) <= 2) is looser than needed.	partly fixed: the torn-line branch is gone in 2074d6d. Open: a test of the empty-file reopen, and the tighter assertion, follow-up FU-P3-10
CR-P3T06-04 (r2)	minor	After 100 earlier errors (easy under ENOSPC, one per event), the cap also suppresses the close-time drop summary.	fixed in 2074d6d: SpoolWriter.finish() writes "N further errors not logged" and the drop summary past the cap (test_error_cap_keeps_the_summary)
CR-P3T06-05 (r2)	minor	The fix changes data-loss behaviour and nobody decided it: under a per-file limit (EFBIG: ulimit -f, a file system's maximum file size), events after the limit are lost for the rest of the run. Before, they spilled into new files.	open, assumed: inbox #82 rule 2; architecture §13.3 says so
CR-P3T06-06 (r2)	minor	The docs still describe the old errorMessage behaviour, and the Nextflow citations in code comments name no version.	docs fixed by this step (api §14.1, §7); code comments open, as CR-P3T06-03 (r1)
SEC-P3T06-01 (r2)	minor	run_local.sh lets Nextflow download nf-spectio@0.0.1 from the public registry when the local unpacked copy is not found. A release published under that ID would run in CI as the plugin under test.	open, follow-up FU-P3-10. The registry ID is now held by spectio-org (inbox #66, answer 2026-10-29), which narrows the risk to a release of our own
SEC-P3T06-02 (r2)	minor	The shutdown hook has no time bound: after 5 s + 10 s it calls the synchronized writer.close() and writer.error(), so a writer blocked holding the monitor (a stalled shared file system, a FIFO as bridge.err or a spool file) keeps the head alive. Reproduced: with .spectio/bridge.err a FIFO, the JVM was alive after timeout's SIGTERM at 60 s; only SIGKILL ended it. Also, after a timed-out join, the writer could create a new spool file after close.	fixed in 2074d6d: when the writer thread is alive after the join, Spool.close() prints one stderr line and returns, without finish() or close(); bridge.err is written only when it is a regular file (test_stuck_writer_does_not_hang_the_head, test_bridge_err_fifo_does_not_hang_the_head). Residual: SEC-P3T06-03 (r3)
SEC-P3T06-03 (r2)	minor	req. 12's bridge rules for the launch directory are not applied (symlinks followed by createDirectories, no owner or mode check, opens by path). Not exploitable in driven runs (0700, req. 5).	open: P1 Task 25, as SEC-P3T06-03 (r1)
SEC-P3T06-04 (r2)	minor	The queue holds up to 100 000 items of about 12 keys plus a trace of up to 41 strings: about 0.5 GB of head heap (estimate, not measured). Hooks never block, but heap pressure can still fail the run.	open, assumed: inbox #82 rule 3
SEC-P3T06-05 (r2)	minor	SEC-P3T1-09 and FU-P3-08 are handled correctly: errorMessage is null whenever session.fault.task is set, the case where Nextflow fills it with the task's stderr or stdout (WorkflowMetadata.groovy:386-395, v26.10.4). The kept text is session.error.message, which can quote values: Nextflow's own errors, a workflow's error() text, compile errors, validation messages [to be validated].	open, assumed: inbox #82 rule 1
CR-P3T06-05 (r3)	minor	run_writer now runs every writer script with capture_output=True and check=True; a failing Groovy script's stack trace is no longer in pytest's failure report.	open, follow-up FU-P3-10
CR-P3T06-06 (r3)	minor	When the join times out, Spool.close() prints the queue-full drop count only, not the events still in the queue, which are lost when the JVM halts. A slow writer (a large backlog on slow NFS) loses the end of the stream and nothing counts it.	open, follow-up FU-P3-10
SEC-P3T06-03 (r3)	minor	The FIFO guard on bridge.err is a check, then a separate open by path. finish() now runs on the shutdown-hook thread, which nothing bounds: a FIFO swapped in between isRegularFile and Files.write hangs the hook, and the head never exits. Only a process of the same user can make the swap (directories 0700); the spool files' check-then-open runs on the writer thread, which the join timeout bounds.	open: P1 Task 25 (fd-relative opens), or a bound on the hook first; inbox #82 rule 3

Closed from earlier reviews:

Id	Outcome
SEC-P3T1-09 (record)	closed for the task's stderr: onFlowComplete.errorMessage is sent as null when a task failed (test_task_stderr_kept_out_of_the_spool). Nextflow's own error text is kept: inbox #82 rule 1
FU-P3-07 (spike first-look)	closed at the source: exitStatus and trace.exit of 2147483647 (Integer.MAX_VALUE, "no exit status yet") are sent as null (test_no_exit_yet_is_never_an_exit_code)
FU-P3-08 (spike first-look)	closed with SEC-P3T1-09: runs.failure.message no longer carries the tool's stderr