Code review · P3 Task 8, spectio init and spectio doctor

review-code on autopilot/P3-t08, base 8bae292, 2026-10-30. The step is P1 Task 8, with the first three tests of gap G6 (P2 §10.7). It adds:

• packages/spectio/src/spectio/config.py: Settings (pydantic-settings), in layers: defaults, spectio.toml, SPECTIO_*, flags; render_toml and write_config;
• packages/spectio/src/spectio/cli/{main,init,doctor}.py: the spectio command (Typer) with init and doctor;
• packages/spectio/src/spectio/loginshell.py: commands through bash -l with a cleared environment (P1 Assumptions 13 and 14);
• packages/spectio-server/src/spectio_server/bootstrap.py: the one server module the CLI imports, inside the command (init_store, db_checks); store/db.py gains preferred_db_dir();
• packages/spectio/tests/test_init.py (18 tests), test_doctor.py (7 tests, 15 cases), conftest.py; one assertion more in spectio-server/tests/test_store.py;
• in pyproject.toml, the two import edges the contracts ignore, and a third contract that keeps the runner off the CLI.

The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. The configuration file and spectio doctor are owned by architecture §7.3; where the data lives, by dev-env §15.3. The rules decided here are in inbox #83.

Round	Head or range	code-reviewer	security-reviewer
1	e41b52a (the step)	changes (0 blockers, 1 major, 6 minors)	changes (0 blockers, 1 major, 5 minors)
2	62fcf09 (the round-1 fix)	approve (0 blockers, 0 majors, 5 minors)	approve (0 blockers, 0 majors, 6 minors)
3	0db5b1a (fixes of six round-2 minors)	approve (0 blockers, 0 majors, 2 minors)	approve (0 blockers, 0 majors, 2 minors)

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

Rounds. One fix round for the two majors, then both approved in round 2. The worker then fixed six round-2 minors (0db5b1a), and round 3 reviewed that fix.

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

Test-first. The worker's record: for 0db5b1a, the six new tests were written first and all six failed before the code changed. Test command, passing at 179b80a:

uv run pytest packages/spectio/tests/test_init.py packages/spectio/tests/test_doctor.py -v

CODE  step=P3-t08  review=approve b/M/m=0/1/13  security=approve b/M/m=0/1/13  rounds=3  simplify=changed -23/+15  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 three changes, one commit each: 83332d1 and 179b80a (CR-P3T08-06 (r1, r2)), bd794b1 (CR-P3T08-07 (r1)). Its after-change run of the default gate: pytest 247 passed, 14 deselected; ruff, ruff format, mypy, lint-imports and uv lock --check clean.

Findings

The finding texts are summarised from the round summaries and the fix commits' messages. A pair such as CR-P3T08-05 (r1) / CR-P3T08-04 (r3) is one topic seen twice.

Id	Severity	Finding	Status
CR-P3T08-01 (r1)	major	engines.extra_args was a flat list[str]. P1 Task 22 writes engines.extra_args.snakemake = [...] in spectio.toml, which then fails validation, so init, doctor and serve would exit on Task 22's config; a flat list also cannot keep Snakemake-only flags away from Nextflow.	fixed in 62fcf09: a table per engine, extra_args = {nextflow = [...], snakemake = [...]}, and SPECTIO_ENGINES_EXTRA_ARGS_NEXTFLOW / _SNAKEMAKE, split as shell words (test_extra_args_per_engine_round_trip)
CR-P3T08-02 (r1)	minor	The first principal is a user that names itself as created_by; no system principal is created. store/schema.py:70-73 says created_by is nullable only so that the first principal, system, can exist. P1 Assumption 18 says "one principal (the Unix user)".	open, assumed: inbox #83 rule 2; the system row is follow-up FU-P3-03
CR-P3T08-03 (r1)	minor	Without the server package, init says "install spectio[server]", but packages/spectio/pyproject.toml declares no server extra.	open, follow-up FU-P3-12 (the extra is architecture §7.3's; P1 Task 21 builds the wheel)
CR-P3T08-04 (r1)	minor	_toml_value sends a bool down the int \| float branch, so repr(True) writes True, which is not TOML. No setting is a bool today; the first one (architecture §7.3 names SPECTIO_TELEMETRY_ENABLED) would make init write a file every later command refuses.	open, follow-up FU-P3-12
CR-P3T08-05 (r1)	minor	spectio.toml is always $SPECTIO_CONFIG or ~/.spectio/spectio.toml. Architecture §7.3 lists $SPECTIO_CONFIG, ~/.config/spectio/, /etc/spectio/; P1 Assumption 21 and dev-env §15.3 put it in the data directory. spectio init --data-dir /x creates both ~/.spectio and /x, and records data_dir = "/x" in a file outside /x.	open, assumed: inbox #83 rule 1; architecture §7.3 has a dated note
CR-P3T08-06 (r1)	minor	simplify: checks() branches on mountinfo is None only to avoid passing the server's default, which duplicates the db_checks call.	applied by code-simplifier in 83332d1: one call; db_checks takes mountinfo: Path \| None
CR-P3T08-07 (r1)	minor	simplify: _free_port ends with an unreachable raise InitError("no free port") marked pragma: no cover.	applied by code-simplifier in bd794b1: one socket, bind(preferred), else port 0
SEC-P3T08-01 (r1)	major	init wrote the resolved default database directory into spectio.toml, so later starts treated it as an explicit --db-dir. That removed the mkdtemp fallback of §15.3 and Assumption 21: once the temporary directory is purged, another user on the login node can block init and serve by creating $TMPDIR/spectio-<uid>. The error message was also wrong.	fixed in 62fcf09: the default is never recorded and is resolved again at each start; doctor resolves it with preferred_db_dir() without creating it, and a squatted default is a warn (test_init_squatted_default_db_dir, test_doctor_default_db_dir). Rest: SEC-P3T08-07 (r2)
SEC-P3T08-02 (r1)	minor	_private_dir rejected only group- or other-write bits, so an existing data directory at 0755 or 0750 was accepted. §15.3 req. 5 wants 0700, and req. 12 makes the bridges write no pointer under a ~/.spectio with any group or other bit. It also used os.stat, which follows symlinks.	fixed in 0db5b1a, as SEC-P3T08-02 (r2)
SEC-P3T08-03 (r1)	minor	spectio.toml holds shell commands (engines.nextflow_cmd, snakemake_cmd) that doctor runs through bash -lc and the head script will run. It is loaded with no owner or mode check: a file at 0666 loads, and doctor says ok config.	open: P1 Task 15 (file modes, §15.3 req. 5), as SEC-P3T08-03 (r2)
SEC-P3T08-04 (r1)	minor	When run_login times out it kills only bash; what it started keeps running. Reproduced: TIMEOUT_S=1 with sleep 77; true returns 124 and leaves sleep 77 running. The runner will reuse this helper (Assumption 13).	open, follow-up FU-P3-13, before P1 Task 9 reuses the helper
SEC-P3T08-05 (r1)	minor	doctor's db_dir check runs the 0700 check and the file-system type only. It skips _check_parents and the realpath re-check of open_db, so it prints ok db_dir for a directory under a parent others can write without the sticky bit, which serve then refuses.	open, follow-up FU-P3-12
SEC-P3T08-06 (r1)	minor	doctor reads $NXF_HOME/config with read_text: it follows symlinks, has no size cap, and blocks on a FIFO. A crafted NXF_HOME can hang doctor or make it read a very large file.	open, follow-up FU-P3-12
CR-P3T08-02 (r2)	minor	The off-Linux refusal looked only at --local. Local mode from SPECTIO_EXECUTOR=local or from executor = "local" in spectio.toml went ahead on darwin.	fixed in 0db5b1a: the check reads the built Settings (test_init_local_from_env_refused_off_linux)
CR-P3T08-03 (r2)	minor	db_checks wrote owner uid {owner} by hand instead of the store's _owner(), so the overflow uid looked like a real owner (the case of SEC-P3T2-14), and an unreadable owner printed "uid None".	fixed in 0db5b1a: both messages use _owner(); test_overflow_uid_is_named checks db_checks too
CR-P3T08-04 (r2)	minor	Two deviations are recorded nowhere: P1's test_init_defaults_unattended says spectio.toml records the database directory (no longer, SEC-P3T08-01), and §15.3 asks for the fallback path in the state file, deferred to Task 11.	fixed by the Progress page: Differences from the plan's interface (Task 8); the state file, SEC-P3T08-07 (r2)
CR-P3T08-05 (r2)	minor	When TMPDIR is unsafe, preferred_db_dir() says so with log.warning. doctor never turns it into a warn line, so it reaches stderr as a raw log record, or is lost.	open, follow-up FU-P3-12
CR-P3T08-06 (r2)	minor	simplify: two branches that differ only in the mountinfo argument, and two if not private blocks that differ only in status and wording.	applied by code-simplifier in 83332d1 and 179b80a
SEC-P3T08-02 (r2)	minor	As SEC-P3T08-02 (r1): init accepts a data directory (and the config directory) at 0755, and a symlink.	fixed in 0db5b1a: _private_dir(path, strict=True) opens with O_DIRECTORY\|O_NOFOLLOW, checks with fstat, and needs this uid and no bit in 0o077; else exit 2 with the owner, the mode and chmod 700 <path> (test_init_refuses_open_or_symlinked_data_dir). The runs root and an explicit --db-dir keep the 0o022 rule. Rest: CR-P3T08-04 (r3), SEC-P3T08-08 (r3)
SEC-P3T08-03 (r2)	minor	As SEC-P3T08-03 (r1); also, $SPECTIO_CONFIG can point at any file. A config file or directory another user can write gives that user code execution as this user.	open: P1 Task 15
SEC-P3T08-04 (r2)	minor	The principal's name came from getpass.getuser(), which reads LOGNAME, USER, LNAME and USERNAME first, so it could be spoofed or wrong (sudo -E, containers).	fixed in 0db5b1a: pwd.getpwuid(os.getuid()).pw_name, else the uid as text (test_principal_is_the_uid_not_the_environment)
SEC-P3T08-05 (r2)	minor	_toml_value used json.dumps(ensure_ascii=True), which writes a character outside the BMP as a surrogate pair that TOML rejects. Such a value made init replace a good spectio.toml with one that cannot load.	fixed in 0db5b1a: ensure_ascii=False, and write_config parses the text with tomllib.loads before it opens the temporary file (test_init_value_outside_the_bmp_round_trips). Rest: CR-P3T08-05 (r3)
SEC-P3T08-06 (r2)	minor	Settings.port accepted 1–1023. §15.3 req. 8 accepts 1024–65535 from server.json, and an unprivileged serve cannot bind below 1024.	fixed in 0db5b1a: Field(8080, ge=1024, le=65535) (test_init_privileged_port_refused)
SEC-P3T08-07 (r2)	minor	Tracking the rest of SEC-P3T08-01: with the default resolved at each start, a squatted $TMPDIR/spectio-<uid> gives a new mkdtemp directory on every start, and nothing records which. §15.3 "Where data lives" says the state file records it.	open: P1 Task 11 (serve, server.json)
CR-P3T08-04 (r3)	minor	The strict 0700 rule now also applies to the directory holding spectio.toml when $SPECTIO_CONFIG points outside the data directory: a 0755 ~/.config/spectio, or a 0755 home with SPECTIO_CONFIG=~/spectio.toml, is refused and the user is told to chmod 700 it. Req. 5 asks this of the data directory only, and spectio.toml holds no secret. Untested.	open, assumed: inbox #83 rule 4
CR-P3T08-05 (r3) / SEC-P3T08-07 (r3)	minor	The worker's report says tomllib.loads(text) catches lone surrogates from the environment. It does not: tomllib.loads('x = "a\ud800b"') returns the string. What stops the write is f.write(text), which raises UnicodeEncodeError once the temporary file exists; the file is removed, spectio.toml is not replaced, and init exits 2 with a raw codec message, after init_store has already run. Fails closed; untested.	open, follow-up FU-P3-12: a clear message, and a test. Reproduced here (Evidence on the Progress page)
SEC-P3T08-08 (r3)	minor	The strict refusal always says chmod 700 <path>, even when the problem is the owner (chmod then fails with EPERM); the overflow uid 65534 shows as a bare "owner uid 65534", bypassing _owner(). The check also duplicates spectio_server.store.db._private_dir, imported a few lines above, so the two rules can drift.	open, follow-up FU-P3-12

Closed from earlier reviews: none. CR-P3T08-02 (r1) joins FU-P3-03 (the system principal, from CR-P3T04-04 (r1), record).