Code review · P3 Task 27, stages declared or detected above the processes list¶
review-code on autopilot/P3-t27, base 855786c, 2026-10-31. The step is P1 Task 27, the only task of milestone M7 (P2 §7), built after Thomas's demo feedback of 2026-10-31 (the order P3-parD, then Tasks 27, 28, 29, 32). It adds:
packages/spectio-server/src/spectio_server/stages/:freeze.py(sources, freeze, assignment, stage state),detect_nf.py,detect_smk.py(withread_confined),assign.py,state.py;packages/spectio-sdk/src/spectio_sdk/stages.py: the 0.0.1spectio.ymlmodel and the overlay match;- the store: table
run_processes(migration0005); the projector freezes atonFlowBeginorworkflow_startedand keeps each process's stage; - the read API:
/stagesfilled,/processesin recorded order,summary.stage_mode,RunDetail.stages_note; - the head script:
snakemake --list-rulesinto.spectio/rules.txtbefore the engine; web/src/runs/RunDetailPage.tsx: the note when declared stages were not applied;- tests:
tests/api/test_stages.py(27 tests, 49 cases), three head-script tests (4 cases), two Playwright checks withtests/e2e/seed_stages.py.
The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. The rules are owned by ui-spec §5.1.1, checking-scenarios §12.7, api §5 and §7; the notes of 2026-10-31 in each are written by the docs commit (inbox #93).
| Round | Head | code-reviewer | security-reviewer |
|---|---|---|---|
| 1 | 8d9b4b4 (the step) |
changes (1 blocker, 1 major, 9 minors, 1 simplify) | changes (1 major, 7 minors) |
| 2 | 38a0946 (Cancel during the listing, deep YAML) |
changes (1 major, 3 minors, 2 simplify) | changes (1 major, 5 minors) |
| 3 | 4fd4bce (Git runs freeze from the fetched copy, YAML aliases refused) |
approve (2 minors) | approve (2 minors) |
| 4 | 125cfb7 (rebuilt Git runs, YAML errors without content) |
approve (2 minors) | approve (2 minors) |
| simplify | 3d4af9b |
code-simplifier: changed, −12 +7 (two commits) |
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: CR-P3T27-04 (r1), (r2), (r3) are three findings.
Test-first. The worker reports that each fix was written as a test first and seen to fail. doc-writer re-checked round 3's: with freeze.py, sink.py and serve.py of 4fd4bce, the two YAML tests (4 cases) fail on the file content in the message, and the three test_snakemake_freeze_git_workflow cases fail on the new data_dir argument (TypeError); the files restored, all 49 cases pass (P3-M7 Evidence). Round 1's Cancel fix has test_cancel_during_the_listing; round 1's deep YAML test_freeze_survives_deep_yaml; round 2's alias bomb test_freeze_refuses_yaml_aliases and test_validation_error_logged_without_input.
Gate at 3d4af9b, from the worktree, re-run by doc-writer:
uv run --locked pytest packages/spectio-server/tests/api/test_stages.py -v # 49 passed
uv run --locked pytest -q # 564 passed, 15 deselected
uv run --locked ruff check . # All checks passed!
uv run --locked ruff format --check . # 91 files already formatted
uv run --locked mypy packages # Success: no issues found in 90 source files
uv run --locked lint-imports # Contracts: 4 kept, 0 broken.
uv lock --check # Resolved 55 packages
cd web && npx vitest run # 18 files, 178 tests passed
cd web && npx tsc --noEmit # no output, exit 0
# smoke, serve-local.sh --seed-stages with Nextflow 26.10.4, desktop and phone:
SPECTIO_E2E_ENGINES=1 SPECTIO_E2E_SEEDED=1 npx --no-install playwright test skeleton.spec.ts --project desktop --project phone
# 20 passed, 6 skipped (the three test.fixme on both projects)
CODE step=P3-t27 review=approve b/M/m=1/2/16 security=approve b/M/m=0/2/16 rounds=4 simplify=changed -12/+7 tests=pass
The counts add up the four rounds per reviewer. The three simplify findings (CR-P3T27-12 r1, CR-P3T27-07 and -08 r2) are not counted.
Findings¶
| Id | Severity | Finding | Status |
|---|---|---|---|
| CR-P3T27-01 (r1) | blocker | A Cancel during the head's snakemake --list-rules (up to 120 s) did not cancel the run: the inner shell traps TERM, the listing dies, the \|\| branch falls through and the engine starts. Only the SIGKILL after the 60 s grace stopped it, which can leave SLURM jobs behind. Reproduced with the exact _LIST_RULES text. |
fixed in 38a0946: timeout --foreground keeps the listing in the head's group, and the inner script checks .spectio/cancel after the listing and exits 143 before the engine; test_cancel_during_the_listing |
| CR-P3T27-02 (r1) | major | Errors other than OSError and ValueError escaped the stage readers: yaml.safe_load(b'[' * 30000) raises RecursionError, so freeze() raised and ingest failed on every tick. |
fixed in 38a0946: RecursionError is a refused file; freeze guards detection and the declared sources, logs, and still records the freeze; test_freeze_survives_deep_yaml |
| CR-P3T27-03 (r1) | minor | When two overlays match, the workflow's own spectio.yml is skipped too and the run goes to detected stages; §12.7 says "none is applied" of the overlays only. |
open: recorded as built in the §12.7 note; the question is inbox #93 |
| CR-P3T27-04 (r1) | minor | Detected stages hold literal names but match with fnmatchcase; a process name with *, ? or [ (only from a forged event of a terminal run) matches other stages' names, and assign raises Overlap in freeze() outside any try. |
open: still so at 3d4af9b (freeze.py:314); follow-up FU-P3-30 |
| CR-P3T27-05 (r1) | minor | An empty or junk rules.txt is taken at face value: an empty listing drops every rule; a word the Snakefile prints at parse time becomes a phantom rule (Snakemake 9.27.1 list_rules prints to the same stdout, workflow.py#L734-L740). |
open (repeated as CR-P3T27-04 r2); follow-up FU-P3-30 |
| CR-P3T27-06 (r1) | minor | The listing's stderr goes to /dev/null, so the logged line does not say why it failed (a timeout, a missing executable, a Snakemake error). |
open: follow-up FU-P3-30 |
| CR-P3T27-07 (r1) | minor | Snakemake stages follow include order; ui-spec §5.1.1 says the topological order of the start rule graph, then include order. No spec note recorded it. | written into the ui-spec §5.1.1 note of 2026-10-31 by the docs commit (assumed, inbox #93 rule 4) |
| CR-P3T27-08 (r1) | minor | On Nextflow, overlay matching uses Spectio's workflow name and workflow_versions.version, not manifest.name and manifest.version as §12.7 says, because nf-spectio sends no manifest. |
written into the §12.7 note of 2026-10-31 by the docs commit (#93 rule 2). doc-writer read runs_write.py:210: the name is the one inspect read at launch, manifest.name when nextflow.config declares one |
| CR-P3T27-09 (r1) | minor | The comment of seed_stages.py says PID 4194303 is "above Linux's default pid_max: never a live process"; on systemd hosts pid_max is 4194304. The seed is safe only because the start time and boot ID would not match. |
open: comment unchanged (seed_stages.py:70); follow-up FU-P3-30 (same as SEC-P3T27-08 r1) |
| CR-P3T27-10 (r1) | minor | test_old_database_opens says old runs' processes "are still listed from their tasks" but checks only that the table appears. |
open: follow-up FU-P3-30 |
| CR-P3T27-11 (r1) | minor | A stage's started_at and ended_at take the batch's last event time (refresh at the end of the transaction), not its first or last task's. |
open: follow-up FU-P3-30 |
| CR-P3T27-12 (r1) | simplify | (a) the "declared wins" loop of _stage_modes; (b) IN_CHUNK and TERMINAL copied from projector.runs; (c) the FROZEN payload's mode and source never read. |
(a) fixed in 089209b, (b) in 3d4af9b, by code-simplifier; (c) left: the payload is stored in events, so dropping it changes data written |
| SEC-P3T27-01 (r1) | major | A deeply nested overlay, spectio.yml or meta.yml raised RecursionError; on Python 3.11 and 3.12 Path.is_file() can raise PermissionError. Either escaped freeze(), so ingest dropped the start event as projection_error; one bad overlay did this to every run. |
fixed in 38a0946, with CR-P3T27-02 (r1); _main_snakefile uses os.path.isfile, which never raises |
| SEC-P3T27-02 (r1) | minor | Nothing caps the stage universe; each name costs a SELECT, a COUNT(*) and an INSERT, plus up to 50 × 200 globs twice, on the single writer thread. |
open (repeated as SEC-P3T27-04 r2 and r3); follow-up FU-P3-30 |
| SEC-P3T27-03 (r1) | minor | freeze() reads its sources synchronously inside writer.submit(); a stalled NFS mount blocks every run's ingest and every API write. Req. 13 (SEC-157) asks this isolation for job files. |
open (repeated as SEC-P3T27-03 r2, SEC-P3T27-04 r3); owner P1 Task 28, which builds req. 13's isolation for job files |
| SEC-P3T27-04 (r1) | minor | read_confined resolves with realpath, then opens the resolved path; O_NOFOLLOW covers the last part only, so a writable intermediate directory swapped for a symlink in between lets the server read outside the directory. |
open (repeated as SEC-P3T27-05 r2); follow-up FU-P3-30 (an fd-relative walk, as workflows._open does) |
| SEC-P3T27-05 (r1) | minor | No test covers snakefile_main from workflow_started; the device case of "FIFO/device refused" is untested. |
partly: test_snakemake_main_outside_pipeline was in 8d9b4b4 (doc-writer read it: a snakefile_main outside the run's directory gives no stages). The device case stays open (SEC-P3T27-06 r2) |
| SEC-P3T27-06 (r1) | minor | timeout 120 has no -k: a listing that traps SIGTERM never ends, so the engine never starts. |
open (repeated as SEC-P3T27-07 r2); follow-up FU-P3-30 |
| SEC-P3T27-07 (r1) | minor | _failed_stage looks up runs.failure.task_id in tasks without filtering on run_id; not exploitable today. |
open: freeze.py:528; follow-up FU-P3-30 |
| SEC-P3T27-08 (r1) | minor | The seed_stages.py PID comment is false on hosts where systemd sets pid_max to 4194304 [to be validated] for the CI runners. |
open: as CR-P3T27-09 (r1) |
| CR-P3T27-03 (r2) | major | Workflows imported from Git got no pipeline directory at the freeze: workflows.location is the URL. Snakemake Git runs never got detected stages or an overlay; Nextflow Git runs lost meta.yml descriptions and their own spectio.yml. |
fixed in 4fd4bce: the directory is the launch command's location (the fetched copy); a Git copy's overlay key is the repository's name; test_snakemake_freeze_git_workflow |
| CR-P3T27-04 (r2) | minor | An empty or noisy rules.txt (as CR-P3T27-05 r1). |
open: FU-P3-30 |
| CR-P3T27-05 (r2) | minor | A Ctrl-C or SIGTERM in the head's tmux during the listing, with no cancel marker, stops only the listing; the engine then starts. Before this step the signal reached the engine. |
open: follow-up FU-P3-30 |
| CR-P3T27-06 (r2) | minor | RunDetail.stages_note is a new field of the public read API that api §5 and §7 do not define. |
written into the api §5 note (bullet of 2026-10-31) by the docs commit (#93 rule 6) |
| CR-P3T27-07 (r2) | simplify | With no rules.txt, the rule files are walked and read twice. |
left by code-simplifier: dropping the first walk changes run_processes.position (the main Snakefile's rules last); a performance fix, FU-P3-30 |
| CR-P3T27-08 (r2) | simplify | _stage_modes guards against two sources per run, which the freeze never writes. |
fixed in 089209b by code-simplifier |
| SEC-P3T27-02 (r2) | major | A YAML alias bomb of about 400 bytes hangs or exhausts the writer: safe_load is linear, but str() of the pydantic ValidationError calls repr() on the shared structure. Measured: 7 levels 1.2 s, 8 levels (414 bytes) 9.4 s, ×9 per level. |
fixed in 4fd4bce: a SafeLoader that refuses aliases, and a ValidationError reported by loc and msg only; test_freeze_refuses_yaml_aliases, test_validation_error_logged_without_input |
| SEC-P3T27-03 (r2) | minor | Sources read inside the ingest transaction on the writer thread (as SEC-P3T27-03 r1). | open: P1 Task 28 |
| SEC-P3T27-04 (r2) | minor | No cap on the number of names at the freeze; O(n²) inserts (as SEC-P3T27-02 r1). |
open: FU-P3-30 |
| SEC-P3T27-05 (r2) | minor | read_confined race (as SEC-P3T27-04 r1). |
open: FU-P3-30 |
| SEC-P3T27-06 (r2) | minor | No refusal tests for a device node, an overlays directory that is group-writable or another uid's, or a group-writable overlay file. The code paths exist (S_ISREG, the fstat check in find_overlay, read_small). |
open: follow-up FU-P3-30 |
| SEC-P3T27-07 (r2) | minor | timeout --foreground 120 with no --kill-after (as SEC-P3T27-06 r1). |
open: FU-P3-30 |
| CR-P3T27-04 (r3) | minor | A Git run of a rebuilt index has no launch command, so its stages change at the rebuild: a Snakemake Git run loses its 8 detected stages; a Nextflow Git run with its own spectio.yml switches to detected. |
fixed in 125cfb7: _git_copy finds <data dir>/pipelines/<parts>/<quote(revision)>, the directory fetch writes, when it is one; third case of test_snakemake_freeze_git_workflow |
| CR-P3T27-05 (r3) | minor | The _rules_txt comment says "a run from a terminal, or a rebuilt one"; a rebuilt run keeps its launch directory and lacks only its runner_commands row. |
open: comment unchanged (freeze.py:447); FU-P3-30 |
| SEC-P3T27-03 (r3) | minor | parse_spectio_yml's ValueError still held file content on malformed YAML (PyYAML's snippet and caret, over several lines) and echoed extra key names; for a Git workflow the file is a third party's. |
fixed in 125cfb7: problem text, line and column only, control characters removed; extra keys as repr(), 40 characters, printable; test_yaml_error_logged_without_content, test_extra_key_reported_on_one_line. Partly: see SEC-P3T27-04 (r4) |
| SEC-P3T27-04 (r3) | minor | The freeze's cost is bounded per file, not per run: 300 names × 2,600 globs take about 1.7 s per pass, twice; detect_nextflow is quadratic in groups; the names from onProcessCreate have no cap; up to 400 meta.yml read on the writer thread. |
open: FU-P3-30, and P1 Task 28 for the thread |
| CR-P3T27-05 (r4) | minor | PyYAML's problem text still quotes file content for some errors (a tag or a tag handle). |
fixed after the workflow, by the orchestrator: _yaml_problem masks every quoted part of problem; two cases (a tag, a %TAG prefix) added to test_yaml_error_logged_without_content, failing first |
| CR-P3T27-06 (r4) | minor | A yaml.reader.ReaderError (bytes that are not UTF-8, a control character) has no problem; the message is just "not YAML: ReaderError". |
open: re-run, below; FU-P3-30 |
| SEC-P3T27-04 (r4) | minor | A tag name from the file reaches the error text and the log: stages: !SECRET-TOKEN-abc123 [] gives "could not determine a constructor for the tag '!SECRET-TOKEN-abc123' (line 1, column 9)"; a %TAG prefix leaks the same way. The docstrings still say the text "never holds the file's content". At most 80 printable characters, so the log cannot be forged. |
fixed after the workflow, by the orchestrator: _yaml_problem masks every quoted part of problem; two cases (a tag, a %TAG prefix) added to test_yaml_error_logged_without_content, failing first |
| SEC-P3T27-05 (r4) | minor | _git_copy calls Path.is_dir(), which raises OSError(ENAMETOOLONG) on Python 3.13 when a path part is too long. check_revision allows 200 characters, and quote(revision, safe='') makes each / three, so one part can pass 255 bytes. _run is outside the try blocks of freeze and of the stage-ended check, so the ingest batch fails. Only for a rebuilt index whose manifest holds such a revision. |
fixed after the workflow, by the orchestrator: _git_copy returns no copy on OSError; test_git_copy_name_too_long failed first (the start event lost), then passes |
Probes re-run by doc-writer on 3d4af9b:
$ uv run --locked python -c "…Path('/tmp/'+'a%2F'*100).is_dir()…"
3.13.14
raises 36 File name too long
$ uv run --locked python -c "…parse_spectio_yml(d) for three inputs…"
"not YAML: could not determine a constructor for the tag '!SECRET-TOKEN-abc123' (line 1, column 9)"
"not YAML: expected ',' or ']', but got ':' (line 2, column 4)"
'not YAML: ReaderError'
Fixes by round¶
| Commit | Findings |
|---|---|
38a0946 fix(stages,head): cancel during --list-rules, freeze survives deep YAML |
CR-P3T27-01 (r1), CR-P3T27-02 (r1), SEC-P3T27-01 (r1) |
4fd4bce fix(stages): Git runs freeze from the fetched copy, YAML aliases refused |
CR-P3T27-03 (r2), SEC-P3T27-02 (r2) |
125cfb7 fix(stages): rebuilt Git runs find their copy, YAML errors carry no content |
CR-P3T27-04 (r3), SEC-P3T27-03 (r3) |
089209b refactor(P3-t27): simplify stage_mode lookup to one dict |
CR-P3T27-12a (r1), CR-P3T27-08 (r2) |
3d4af9b refactor(P3-t27): simplify stages constants to projector.runs imports |
CR-P3T27-12b (r1) |
code-simplifier reran the step's check command before and after its changes (pytest 564 passed, 15 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 27 page is written: P3-M7, with its findings and open questions.