Code review · P3 Task 11, spectio serve bound to loopback, boot and rebuild

review-code on autopilot/P3-t11, base c16ea57, 2026-10-30. The step is P1 Task 11, built in the local-only order of inbox #80 (a), before Tasks 10 and 12. It adds:

• packages/spectio-server/src/spectio_server/serve.py: the start (bind check, runs root check, serve.lock, the recorded port, server.json), boot(), the start text and the clean stop;
• packages/spectio-server/src/spectio_server/rebuild.py: rebuild_index, the run directories missing from the index, under dev-env §15.3 req. 10, with per-boot caps;
• packages/spectio-server/src/spectio_server/sink.py: StoreSink, the server's RunnerSink through the one writer;
• packages/spectio-server/src/spectio_server/app.py: create_app, one plain page, no data, no OpenAPI docs;
• packages/spectio/src/spectio/cli/serve.py and cli/main.py: spectio serve [--force]; config.py: [server] host, SPECTIO_SERVER_HOST and LOOPBACK;
• bootstrap.py: ensure_tenancy, public for the boot; fastapi and uvicorn as dependencies; one more ignored import edge in pyproject.toml;
• packages/spectio-server/tests/test_serve.py, tests/security/test_req1_bind.py, tests/conftest.py.

The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. The bind, the state file, where data lives and the rebuild are owned by dev-env §15.3; the head's detach by schedulers §3.1.1.

Round	Head or range	code-reviewer	security-reviewer
1	ebaa6e4 (the step and inbox #85)	changes (0 blockers, 1 major, 3 minors)	approve (0 blockers, 0 majors, 8 minors)
2	62bb506 (root-only guard)	approve (0 blockers, 0 majors, 5 minors, 1 simplify)	approve (0 blockers, 0 majors, 5 minors)
3	b9b94f2 (workflow source, failure message)	approve (no finding)	approve (0 blockers, 0 majors, 1 minor)
simplify	fd2ae94	code-simplifier: one change, −2 +2

IDs repeat across rounds. Each round numbered its findings again, so each ID below carries its round: CR-P3T11-04 (r1) is not CR-P3T11-04 (r2). The commit messages use the bare IDs of the round they answer. The reports were relayed by the coordinator in summary.

Test-first. For the round-2 fixes (b9b94f2), both tests were run against the code before the fix and failed for the reason they name:

• test_rebuild_one_engine_per_location (extended): a Nextflow run at a git URL without a revision got a second workflows row, with source local;
• test_rebuild_failed_launch_without_message (new): a launch-failed.json with only class stored the message 'None'.

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

uv run --locked pytest -q            # 333 passed, 15 deselected
uv run --locked ruff check .         # All checks passed!
uv run --locked ruff format --check . # 57 files already formatted
uv run --locked mypy packages        # Success: no issues found in 56 source files
uv run --locked lint-imports         # Contracts: 4 kept, 0 broken.
uv lock --check                      # Resolved 43 packages

The session runs as uid 0, so the two root-only tests (test_rebuild_skips_foreign_dir, test_runs_root_foreign_refused) ran here. In CI, as the non-root runner user, they are skipped (CR-P3T11-01 (r1)).

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

The counts add up the three rounds per reviewer; the one simplify finding of round 2 (CR-P3T11-05 (r2)) is not counted as a minor.

Findings

Id	Severity	Finding	Status
CR-P3T11-01 (r1)	major	Two new tests chown to another uid with no root guard. CI's product job runs as the non-root runner user, where chown(2) fails with EPERM, so both tests would fail on the PR.	fixed in 62bb506: the two foreign-owner cases are their own tests, skipped unless the effective uid is 0; the symlink, loose and bad-name cases stay unconditional
CR-P3T11-02 (r1)	minor	The per-boot caps do not bound refused entries: used counts only parsed manifests, so a runs root with many bad run directories is read and logged again at every boot. The byte count is len(manifest.model_dump_json()), not the bytes read.	open, follow-up FU-P3-15
CR-P3T11-03 (r1)	minor	A server.json that names this node is replaced without checking that the server it records is dead. Two servers on one node with different db_dir (SPECTIO_DB_DIR, another $TMPDIR, the mkdtemp fallback) and different ports both start, with two runner loops over one runs root.	open, P1 Task 15 (it also changes how the state file is created, SEC-P3T11-02 (r1))
CR-P3T11-04 (r1)	minor	On a SLURM environment, the rebuild writes the local launcher's native ID <pid>.<start> into launches.head_native_id. Reconciled rows are not visited again, so the tmux launcher inherits IDs in a form it may not use.	open, P1 Task 10 (the tmux native ID)
SEC-P3T11-01 (r1)	minor	serve.lock is created, truncated and written before the parents of db_dir are resolved and checked (the SEC-P3T2-13 control in open_db).	open, P1 Task 15
SEC-P3T11-02 (r1)	minor	The first server.json is opened with O_EXCL on its final name and written in place. Req. 5 asks for a temporary name and a rename, so another node can read an empty or partial file.	open, P1 Task 15 (req. 5); see SEC-P3T11-02 (r2)
SEC-P3T11-03 (r1)	minor	Another user on the node can hold idle connections to the loopback port with no time limit: uvicorn's h11 protocol starts no timeout before the first response. The runner shares the process, so running out of descriptors also stops spool reading.	open, P1 Task 12 (the first task that serves to a session) [to be validated] against uvicorn's timeouts
SEC-P3T11-04 (r1)	minor	Req. 2 (session on every request) and req. 4 (Host allow-list) are not in place, as instructed. The placeholder answers any client and Host, and nothing stops a data route from being added before Task 12's middleware.	open, P1 Tasks 12 and 14; inbox #85: Task 12 before any data route
SEC-P3T11-05 (r1)	minor	The dependency floors allow vulnerable transitive versions: uvicorn>=0.32 declares h11>=0.8, but CVE-2025-43859 is fixed only in h11 0.16.0; starlette before 0.49.1 has CVE-2025-62727. uv.lock pins safe versions; the published wheel's metadata does not.	open, P1 Task 21 (the published wheel); see SEC-P3T11-03 (r2)
SEC-P3T11-06 (r1)	minor	A manifest that fails validation is logged with %r of the pydantic ValidationError, which carries input_value excerpts: parameter values from run.json reach the server log.	open, follow-up FU-P3-15
SEC-P3T11-07 (r1)	minor	Rebuilt runs whose head is alive or ended get the state queued, which elsewhere means "not launched yet". No test pins that a manifest on disk can never lead to a launch.	open, P1 Task 17 (the launch path, where queued gains its command)
SEC-P3T11-08 (r1)	minor	_write_state checks the data directory with open_private(data_dir, ()), which refuses only group and other write bits: a 0755 data directory is accepted, but req. 5 and Task 8's rule ask for 0700.	open, P1 Task 15 (req. 5)
CR-P3T11-02 (r2)	minor	The "another owner" case of test_runs_root_loose_refused (a row of the plan's test list) never runs in CI. It could, with the repository's existing monkeypatch of the uid.	open, follow-up FU-P3-16
CR-P3T11-03 (r2)	minor	In local mode the start text has no tunnel line, though item (3) of the step asked for one; a local executor on a remote machine reached over ssh gets no hint.	not changed: G6's test_serve_local_start_text asks for no ssh -N -L line in local mode
CR-P3T11-04 (r2)	minor	The rebuild infers a workflow's source from whether a revision is set: a git workflow without a revision becomes local and gets a second workflows row.	fixed in b9b94f2: an absolute path is local (plan D2), anything else git
CR-P3T11-05 (r2)	simplify	ENGINES repeats the Engine literal of spectio.runner.types.	fixed in fd2ae94 by code-simplifier: ENGINES = get_args(Engine)
CR-P3T11-06 (r2)	minor	A launch-failed.json with no message key stores the string 'None'.	fixed in b9b94f2: "The launch failed (index rebuilt)."
CR-P3T11-07 (r2)	minor	serve.py imports the private _require_private_dir from spectio_server.store.db. The call is needed (the lock file is created before open_db checks the directory); the helper is used across modules.	open, follow-up FU-P3-16
SEC-P3T11-01 (r2)	minor	After the CR-P3T11-01 (r1) fix, CI tests no "another user's directory" refusal of req. 10: both foreign-uid tests are skipped there, and no other suite checks the uid comparison of open_private and read_small without root.	open, follow-up FU-P3-16 (with CR-P3T11-02 (r2))
SEC-P3T11-02 (r2)	minor	The first start writes server.json in place (O_CREAT|O_EXCL on the final name, then write(), no fsync). The mode is right from creation; a crash or a concurrent reader can see an empty or partial file, which fails closed today and nudges the user to --force.	open, P1 Task 15 (req. 5)
SEC-P3T11-03 (r2)	minor	fastapi>=0.115 allows starlette releases before the 2025 DoS fixes (CVE-2025-54121, CVE-2025-62727) [to be validated against the advisories]; uvicorn>=0.32 does not force h11>=0.16 (CVE-2025-43859). The lock is fine: h11 0.16.0, starlette 1.10.0, uvicorn 0.54.0, fastapi 0.162.0, all with hashes.	open, P1 Task 21
SEC-P3T11-04 (r2)	minor	Req. 10's "replays a capped number of bytes": the rebuild caps directories and manifest bytes, but the first loop.tick() reads the spool of every non-terminal launch, and MAX_READ_BYTES (2 MiB) is per launch, so one boot may replay about 1000 × 2 MiB before uvicorn starts. The data is the user's own: an availability and boot-time risk.	open, follow-up FU-P3-15
SEC-P3T11-05 (r2)	minor	The loopback port is reachable by every Unix user of a shared login node, with no session (req. 2) and no Host allow-list (req. 4). Only GET / with the version is served today. Inbox #85 says Task 12 must be merged before any data route, but nothing enforces it.	open, P1 Tasks 12 and 14; inbox #85
SEC-P3T11-01 (r3)	minor	The rebuild writes any non-absolute location as a git workflow without checking the import rule of D2 and api §5 (https or ssh URL, or owner/repo; no file://, no ext::, no credentials in the URL). A malformed or planted manifest can leave a row such as ext::sh -c … with source git, which a later step may trust.	open, P1 Task 17: the validator of G5 is applied to rebuilt rows too

Closed from earlier reviews: SEC-P3T08-07 (r2), "which mkdtemp directory a start used is not recorded": server.json now records db_dir.

Round 2 fixes

On autopilot/P3-t11, after ebaa6e4:

Commit	Findings
62bb506 test(server): root-only guard on the foreign-uid serve tests	CR-P3T11-01 (r1)

Round 3 fixes

On autopilot/P3-t11, after 62bb506:

Commit	Findings
b9b94f2 fix(server): rebuild infers workflow source from location; default launch failure message	CR-P3T11-04 (r2), CR-P3T11-06 (r2)
fd2ae94 refactor(P3-t11): simplify ENGINES, derived from the Engine literal	CR-P3T11-05 (r2), by code-simplifier after round 3

code-simplifier left three things alone: the unused settings argument of create_app (a public signature), the cleanup in boot() that repeats Booted.close() (it runs before Booted exists), and two if workflow is not None branches in _add (about one line saved).

For the Progress page

The Task 11 section of P3-M3 is written: P3-M3 · Task 11, with its findings and open questions.