Code review · P3 Task 28, each job's files and the work directory¶

review-code on autopilot/P3-t28, base 3a103a9, 2026-10-31. The step is P1 Task 28, the first task of milestone M8 (P2 §8), a security boundary (confinement, req. 13). The brief also gave it SEC-P3T27-03 (stage sources read off the writer thread). It adds:

packages/spectio-server/src/spectio_server/files/: confine.py (the fd-relative walk of checking-scenarios §11.1), pool.py (the file pool with a timeout), redact.py (the .command.run lexer and strip_escapes), taskfiles.py (the allow-list, the listing states, the path checks for events);
api/taskfiles.py: GET /tasks/{id}/attempts/{n}/files and …/files/{name}; api/logs.py rewritten on the same walk and pool;
the store: attempts.work_dir, attempts.native_id, attempts.logs (migration 0006); the projector keeps each path only under a root of the run and counts the others;
the SDK: snakemake.slurm_submitted and job_error.aux.external_jobid;
the stage freeze reads its sources in a file pool, 10 s at most;
the head script: NXF_LOG_FILE, unset NXF_DEBUG, --slurm-keep-successful-logs; nf-spectio: workdir and native_id on every task event, enableMetrics() true;
tests: tests/test_taskfiles.py (23 tests), tests/api/test_taskfiles_api.py (18 tests, 20 cases), test_serve.py::test_boot_freeze_pool_apart, two engine tests in packages/spectio/tests/test_bridge_nextflow.py.

The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. The rules are owned by dev-env §15.3 req. 13, architecture §13.7, api §5, §7 and §14.1, and checking-scenarios §11.1; the notes of 2026-10-31 in each are assumed under inbox #94.

Round	Head	code-reviewer	security-reviewer
1	`2465328` (the step)	changes (1 major, 5 minors, 2 simplify)	changes (1 blocker, 6 minors)
2	`297ba97` (quoted export words, arrays, whole here-document bodies)	changes (1 major, 4 minors)	changes (1 major, 6 minors, 1 simplify)
3	`f362fc5` (no link followed for a job file)	approve (1 minor)	approve (6 minors)
4	`0fee201` (nesting limit, OSError mapping, name limits, the freeze's own pool)	approve (4 minors)	approve (2 minors)
simplify	`d4f41fc`	`code-simplifier`: changed, −13 +1 (one commit)

The reports were relayed by the coordinator in summary. Finding IDs restart in each round, so the same ID can name different findings; this record writes the round after each ID: SEC-P3T28-04 (r1), (r2), (r3) are three reports of one problem, CR-P3T28-03 (r1), (r2), (r3) three different findings.

Test-first. The worker reports that each fix was written as a test first and seen to fail. doc-writer re-checked the last one (0fee201): with the five source files of f362fc5 put back, the five new tests (7 cases) fail, and with HEAD restored all 7 pass (P3-M8 Evidence).

Gate at d4f41fc, from the worktree, re-run by doc-writer:

uv run --locked pytest packages/spectio-server/tests/test_taskfiles.py \
    packages/spectio-server/tests/api/test_taskfiles_api.py -v   # 43 passed
uv run --locked pytest -q             # 611 passed, 17 deselected
uv run --locked ruff check .          # All checks passed!
uv run --locked ruff format --check . # 100 files already formatted
uv run --locked mypy packages         # Success: no issues found in 99 source files
uv run --locked lint-imports          # Contracts: 4 kept, 0 broken.
uv lock --check                       # Resolved 55 packages
# engine tests, Nextflow 26.10.4 (SHA-256 = NEXTFLOW_SHA256 of dev/versions.env):
uv run --locked pytest packages/spectio/tests/test_bridge_nextflow.py -m engines -v

The engine result is on P3-M8. P1 Task 28 Step 4 on slurm-mini is not run: Task 22 is not built.

CODE  step=P3-t28  review=approve b/M/m=0/2/14  security=approve b/M/m=1/1/20  rounds=4  simplify=changed -13/+1  tests=pass

The counts add up the four rounds per reviewer. The three simplify findings (CR-P3T28-07 and -08 r1, SEC-P3T28-09 r2) are not counted.

Findings¶

Id	Severity	Finding	Status
CR-P3T28-01 (r1)	major	The `.command.run` redaction showed the value of a quoted `export` or `declare` word and of an array assignment: `export "TOKEN=abc"`, `export 'TOKEN=abc'` and `arr=(sec1 sec2)` were served unchanged. Req. 13 says every `export` value is hidden.	fixed in `297ba97`: a quote that opens the whole word after a declaring word is skipped, an array value is read to its `)` and hidden whole. `doc-writer` probe on `d4f41fc`: the three give `export TOKEN=«hidden»` twice and `arr=«hidden»`
CR-P3T28-02 (r1)	minor	An `OSError` other than `ENOENT`, `ELOOP`, `ENOTDIR`, `EACCES`, `EPERM` or `ENXIO` in the task-file routes gave a 500.	fixed in `0fee201` with SEC-P3T28-04 (r3)
CR-P3T28-03 (r1)	minor	`test_req13_spectio_dir` writes `.spectio/nextflow.log` but never asserts it is served; `test_req13_freeze_reads_off_the_writer` checks only the time and the run state.	open: both still so at `d4f41fc` (`test_taskfiles_api.py:281-293`, `:348-370`). `.spectio/nextflow.log` is asserted by `test_engine_log_from_the_run`. Follow-up FU-P3-31
CR-P3T28-04 (r1)	minor	The built interface differs from the plan's (`Root` in place of an fd, `work` optional, extra functions), recorded nowhere.	recorded by the docs commit: P3-M8, Differences from the plan's interface. P1 is edited at the next `/apply-findings`
CR-P3T28-05 (r1)	minor	api §7 puts `native_id` on `scheduler_jobs`, while the code adds `attempts.work_dir`, `native_id` and `logs` with no §7 note; the §5 note says the screens read `n` "from the task's last attempt", which no route or field gives.	fixed by the docs commit: a dated note under the api §7 table, and the §5 note now says no route gives a task's attempt numbers yet, for P1 Task 32 to add
CR-P3T28-06 (r1)	minor	FU-P3-30 and inbox #93 say to apply option (b) at the next change of `stages/freeze.py`; this step changed that file and left it.	open, rightly outside Task 28's scope; recorded on P3-M8. FU-P3-30 keeps it
CR-P3T28-07 (r1)	simplify	`opened_root` is never used and opens its root without `O_NOFOLLOW`.	fixed in `d4f41fc` by `code-simplifier`
CR-P3T28-08 (r1)	simplify	`api.taskfiles` re-exports `FilePool` in `__all__` only for a test.	open: left by `code-simplifier` (a test of kept code would change, `test_taskfiles_api.py:362`). FU-P3-31
SEC-P3T28-01 (r1)	blocker	The fallback that hides everything from the first unreadable line showed the here-document lines before the failing line, unredacted: the container environment's `export` values.	fixed in `297ba97`: a here-document body that cannot be read is hidden from its first line (`redact.py:206-207`)
SEC-P3T28-02 (r1)	minor	A Nextflow job file that is a symlink is followed into the run's work directory: `.command.err` → `.command.env`, `.command.log` → `.command.run` unhidden; the listing says "not a regular file".	fixed in `f362fc5` (raised again as a major in r2)
SEC-P3T28-03 (r1)	minor	The lexer does not hide `docker run -eTOKEN=v` nor `function f { TOKEN=v; }`.	open (raised again as SEC-P3T28-03 r2 and r3); probe below; FU-P3-31
SEC-P3T28-04 (r1)	minor	An unexpected `OSError` (EIO, ESTALE, EINVAL from `readlink`) gives a bare 500 on both task-file routes.	fixed in `0fee201` with SEC-P3T28-04 (r3)
SEC-P3T28-05 (r1)	minor	No limit on the Snakemake log paths kept per attempt; `_logs` removes duplicates in O(n²) on the writer thread; each listing stats every path.	open (again as SEC-P3T28-05 r2): `projector/snakemake.py` `_logs` has no cap at `d4f41fc`. FU-P3-31
SEC-P3T28-06 (r1)	minor	The writer still waits for the freeze, up to 10 s; with the pool busy with route reads, a freeze that times out is final.	partly fixed in `0fee201` (SEC-P3T28-06 r3): the freeze has its own pool. The 10 s bound and the permanent loss on a real stall remain, stated in dev-env §15.3 req. 13 and on P3-M7
SEC-P3T28-07 (r1)	minor	`strip_escapes` keeps the Unicode bidi and format controls (U+202A–U+202E, U+2066–U+2069, U+200E/F).	open (again as SEC-P3T28-08 r3); probe below; FU-P3-31
CR-P3T28-02 (r2)	major	A Nextflow job file read follows a symlink into the run's work directory, so `.command.err` linking to another task's `.command.env` or a staged input serves that file; the listing reports it "Not served".	fixed in `f362fc5`: `read_task_file` calls `open_confined(root, rel)` with no work root, as the Snakemake branch did; `test_req13_job_file_symlink_in_work_refused`, `test_req13_job_file_symlink_422`
CR-P3T28-03 (r2)	minor	After a declaring word, an assignment whose name part is quoted is not hidden: `export "TOKEN"=abc1`, `export TOKEN"=abc2"`, `export 'T'='abc3'`.	open: all three served unchanged at `d4f41fc` (probe below). FU-P3-31
CR-P3T28-04 (r2)	minor	`_in_pool` maps no plain `OSError` (as CR-P3T28-02 r1).	fixed in `0fee201`
CR-P3T28-05 (r2)	minor	`test_req13_freeze_reads_off_the_writer` does not check the frozen run has no stages and the note "The stage sources were not read in time"; P3-M7 cites it to close SEC-P3T27-03.	open (as CR-P3T28-03 r1, second half). FU-P3-31
CR-P3T28-06 (r2)	minor	The interface differences (as CR-P3T28-04 r1), now also `list_task_files(AttemptFiles)`.	recorded by the docs commit, as CR-P3T28-04 (r1)
SEC-P3T28-02 (r2)	major	Nextflow job files follow a final symlink into the work directory (as CR-P3T28-02 r2).	fixed in `f362fc5`
SEC-P3T28-03 (r2)	minor	Not hidden: `exec env T=v`, `command export T=v`, `nohup env T=v`, `sudo T=v cmd`, `docker run -eT=v`, `ch-run --set-env=T=v`, `2>/dev/null T=v cmd`.	open: all served unchanged at `d4f41fc` (probe below). FU-P3-31
SEC-P3T28-04 (r2)	minor	Deep nesting in `.command.run` raises `RecursionError`, a 500; 250 nested here-documents take about 5 s per request.	fixed in `0fee201` with SEC-P3T28-04 (r3)
SEC-P3T28-05 (r2)	minor	`attempts.logs` grows without bound from untrusted events; quadratic projection; the listing stats every path.	open: as SEC-P3T28-05 (r1). FU-P3-31
SEC-P3T28-06 (r2)	minor	A full pool or a stalled source freezes the run with no stages, for good.	partly fixed in `0fee201`, as SEC-P3T28-06 (r1)
SEC-P3T28-07 (r2)	minor	The `.spectio/` exclusion compares names case-sensitively; on a case-insensitive file system `.Spectio/manifest.json` passes `launch_rel` and `check_name`.	open: `taskfiles.py` `launch_rel` and `check_name` compare `".spectio"` exactly at `d4f41fc`. FU-P3-31
SEC-P3T28-08 (r2)	minor	Unmapped `OSError`s (EIO, ESTALE, ENAMETOOLONG, EINVAL) give a bare 500.	fixed in `0fee201`
SEC-P3T28-09 (r2)	simplify	`opened_root` is never called and checks neither owner nor mode.	fixed in `d4f41fc`, with CR-P3T28-07 (r1)
CR-P3T28-03 (r3)	minor	The rule "a job file is never read through a link, not even one into the work directory" is only in a code comment; checking-scenarios §11.1 describes the generic walk as following one.	fixed by the docs commit: written into the dev-env §15.3 req. 13 note of 2026-10-31 and inbox #94 rule 7
SEC-P3T28-03 (r3)	minor	Values still visible: scheduler directive comments (`#SBATCH --export=ALL,TOKEN=s3cret`, `#PBS -v`, `#$ -v`, `#BSUB -env`), `function f { TOKEN=s3; }`, `builtin export T=s3`, `exec env`, `nohup env`, `sudo env T=s3 cmd`, `-eTOKEN=s3`.	open (probe below). The plan keeps `#SBATCH` lines on purpose (`test_req13_command_run_redacted`); hiding an `--export=` list in them is a choice for Thomas, inbox #94 rule 8. FU-P3-31
SEC-P3T28-04 (r3)	minor	(1) About 300 or more nested `$(`, `${`, `"${` or here-documents raise `RecursionError`; (2) EIO, ESTALE and ENAMETOOLONG reach the client as a 500 with a traceback.	fixed in `0fee201`: `MAX_NESTING = 64`, past it `_Unreadable` (hidden from that line); `_in_pool` maps EIO and ESTALE to `503 runner-offline`, other errors to `404 log-not-found`, fixed text; `safe_rel` refuses a component over 255 bytes or a name over 4096. `test_req13_redact_deep_nesting_hidden`, `test_req13_deep_wrapper_served_hidden`, `test_req13_os_errors_mapped` (3 cases), `test_req13_long_log_names_dropped`
SEC-P3T28-05 (r3)	minor	`open_confined(…, work=…)` follows a final link into the work directory and serves whatever it reaches, `.command.env` included. No product caller passes `work=` since `f362fc5`, but P1 Task 31 reuses the module for `publishDir` results.	open, latent: for P1 Task 31, which must refuse the req. 13 "Not served" names at the link target. FU-P3-31
SEC-P3T28-06 (r3)	minor	The freeze shares the 4 + 16-slot pool with the routes, and a `Stalled` freeze is permanent; 20 stalled runs can hold the writer up to 200 s.	partly fixed in `0fee201`: `boot()` makes `FilePool(FREEZE_WORKERS=2, backlog 0)` for the freeze, so route reads never take its slots (`test_boot_freeze_pool_apart`). Left: a freeze that stalls still holds the writer up to 10 s, and once both freeze threads are stuck a freeze is refused at once; a lost freeze is not retried. The retry option was not taken (it would call the freeze from other events, and each retry holds the writer 10 s more). Stated in the req. 13 note and on P3-M7
SEC-P3T28-07 (r3)	minor	The foreign-uid tests run only as root: `test_req13_foreign_uid` and the device branch of `test_req13_symlink_fifo_refused` skip on CI (non-root).	open: here, as root, `test_req13_foreign_uid` passed. CI coverage of the owner check needs a root job or a second user. FU-P3-31
SEC-P3T28-08 (r3)	minor	`strip_escapes` keeps the bidi controls (U+202A–202E, U+2066–2069, U+200E/F, U+061C) and U+2028/2029; an 8-bit CSI (`\x9b31m`) leaves `31m`.	open: probe below. FU-P3-31
CR-P3T28-01 (r4)	minor	The OSError fix covers the task-file routes only: the run-log route still puts the system message in its 404 and maps EIO and ESTALE to 404, not 503. The pool-error mapping exists twice.	open: `logs.py:89-90` at `d4f41fc`. The req. 13 note now says which routes follow the rule. FU-P3-31
CR-P3T28-02 (r4)	minor	Every `OSError` other than EIO and ESTALE becomes a silent 404 with nothing logged: EMFILE, ENFILE, ENOMEM look like a missing file.	open: FU-P3-31 (log the errno, keep the fixed text)
CR-P3T28-03 (r4)	minor	In `test_boot_freeze_pool_apart`, `pytest.raises(Stalled)` on the routes' pool passes whether or not the 20 readers hold its slots, and the test depends on `time.sleep(0.2)`.	open: FU-P3-31
CR-P3T28-04 (r4)	minor	Inbox #94 rule 3 still describes a single pool; its Assumed line says rules 1 to 6 hold as built.	fixed by the docs commit: rule 3 now gives the two pools, the 10 s writer bound and the OSError mapping. #94 is this step's own entry and Thomas has not answered it, so its context is corrected, not its options
SEC-P3T28-07 (r4)	minor	`safe_rel` measures a name with `surrogatepass` but does not check the file system can encode it: a lone surrogate passes, is stored, and the whole listing raises `UnicodeEncodeError` (a 500). Not reachable through the spool today: pydantic-core's JSON parser rejects lone surrogates [to be validated].	open: FU-P3-31
SEC-P3T28-08 (r4)	minor	The run-log route does not follow the OSError rule of the req. 13 note: every OSError is 404 with the system message.	open: as CR-P3T28-01 (r4)

Probes re-run by doc-writer on d4f41fc (redact_command_run on one line each, strip_escapes on the r3 input):

'export "TOKEN=abc"' -> 'export TOKEN=«hidden»'
"export 'TOKEN=abc'" -> 'export TOKEN=«hidden»'
'arr=(sec1 sec2)' -> 'arr=«hidden»'
'export "TOKEN"=abc1' -> 'export "TOKEN"=abc1'
'export TOKEN"=abc2"' -> 'export TOKEN"=abc2"'
"export 'T'='abc3'" -> "export 'T'='abc3'"
'#SBATCH --export=ALL,TOKEN=s3cret' -> '#SBATCH --export=ALL,TOKEN=s3cret'
'function f { TOKEN=s3; }' -> 'function f { TOKEN=s3; }'
'builtin export T=s3' -> 'builtin export T=s3'
'exec env T=s3 cmd' -> 'exec env T=s3 cmd'
'nohup env T=s3 cmd' -> 'nohup env T=s3 cmd'
'sudo T=s3 cmd' -> 'sudo T=s3 cmd'
'docker run -eTOKEN=s3 img' -> 'docker run -eTOKEN=s3 img'
'2>/dev/null T=s3 cmd' -> '2>/dev/null T=s3 cmd'
'ch-run --set-env=T=s3 img' -> 'ch-run --set-env=T=s3 img'
deep $( x2000: 'export A=«hidden»\n«hidden»\n'
strip: 'ared31m‮evil⁦x'

The first three are the r1 fixes. The next twelve are the open redaction findings: each serves a value. The deep case is the r3 fix: hidden from the line that nests too deep, no 500.

Fixes by round¶

Commit	Findings
`297ba97` fix(server): hide quoted export words, array values and whole here-document bodies	CR-P3T28-01 (r1), SEC-P3T28-01 (r1)
`f362fc5` fix(server): never follow a link when reading a Nextflow job file	CR-P3T28-02 (r2), SEC-P3T28-02 (r1, r2)
`0fee201` fix(server): refuse deep wrappers and long names cleanly, give the freeze its own pool	SEC-P3T28-04 (r1, r2, r3), CR-P3T28-02 (r1), CR-P3T28-04 (r2), SEC-P3T28-08 (r2); SEC-P3T28-06 (r1, r2, r3) in part
`d4f41fc` refactor(P3-t28): simplify confine by removing the unused opened_root	CR-P3T28-07 (r1), SEC-P3T28-09 (r2)

code-simplifier reran the step's check command before and after its change (pytest 611 passed, 17 deselected; ruff, ruff format, mypy, lint-imports, uv lock --check): the same results, apart from the time uv lock printed.

For the Progress page¶

The Task 28 section is written: P3-M8, with its findings and open questions.