Code review · T5, scenario runner and isolation

review-code on autopilot/T5, head 5497cfc, 2026-10-28. The step adds dev/scenario.py (up, down, list), the scenario files and unit tests in tests/scenarios/, the isolation proof tests/scenarios/parallel.sh and its workflow scenarios.yml, and changes dev/k8s/cluster.sh for per-scenario kind clusters. Reviewers: code-reviewer and security-reviewer, independent, in parallel. Format: README.md.

Round	Head	code-reviewer	security-reviewer
1	5497cfc	changes (0 blockers, 1 major, 6 minors)	changes (0 blockers, 2 majors, 5 minors)
2	4148643 (the round-1 fixes, delta 5497cfc..4148643)	changes (1 blocker, 0 majors, 4 minors)	approve (0 blockers, 0 majors, 5 minors)
3	aa05608 (the round-2 fixes, delta 4148643..aa05608, code only)	changes (0 blockers, 1 major, 1 minor)	not run
4	aaff4e0 (the round-3 fixes, PR #43, code only)	approve (0 blockers, 0 majors, 2 minors)	not run

The fixes are in the commit that adds this page (d0d34cb) and in e347ff4, on autopilot/T5, and were reviewed in round 2. The round-2 fixes are in the commit that adds the round-2 rows and were reviewed in round 3; the round-3 fixes (PR #43, squash aaff4e0, every check green on 5158f4c, scenarios.yml included) were reviewed in round 4. code-simplifier was not run. docker, sind and kind cannot run in a Claude Code cloud session (cgroup v1), so the fixes were proved in CI on e347ff4: scenarios.yml 40688967336 and pipelines-k8s.yml 40688967168 green. d0d34cb alone broke pipelines-k8s.yml (40688649443: cluster.sh set its data directory before dev/lib.sh defined SPECTIO_REPO). shellcheck -x and bash -n pass on dev/k8s/cluster.sh and tests/scenarios/parallel.sh. Test command:

python3 -m unittest discover -s tests/scenarios -p 'test_*.py'

CODE  step=T5  review=approve b/M/m=1/2/13  security=approve b/M/m=0/2/10  rounds=4  simplify=notrun -0/+0  tests=pass

Id	Severity	Finding	Status
CR-1	major	A failed up left a partial environment (network, realm or cluster, data directory), and the error message printed a literal <id>	fixed in the commit that adds this page: after the ID is claimed, any exception in up runs down for that ID and is re-raised; the message no longer names a placeholder; unit test
CR-2	minor	down deleted the data directory even when deleting the realm or cluster failed	fixed in the commit that adds this page: the data directory is kept when a step fails, so the ID stays claimed; unit test. A follow-up commit also keeps it when sind or kind is missing from the host PATH for a scenario whose scenario.json names a SLURM or Kubernetes scheduler (a realm or cluster would otherwise leak silently); unit test
CR-3	minor	--id 0 was read as "no ID" and took the lowest free one	fixed in the commit that adds this page: n is not None in claim; ID 0 is rejected; unit test
CR-4	minor	No test of claim, the atomic ID claim	fixed in the commit that adds this page: 8 threads calling claim(None) get 8 distinct IDs, claim(3) twice exits; STATE points at a temporary directory
CR-5	minor	The slurm-site, slurm-rest and k8s-kueue profiles are not exercised by CI	kept, recorded: dev-env §5 "Built in T5" and T5 Findings item 8
CR-6	minor	parallel.sh required kind get clusters to equal spx-3, had no cleanup on failure, and did not say it is CI-only	fixed in the commit that adds this page: grep -qx spx-3, an EXIT trap that runs down for IDs 1-4 on failure, and a header that says CI only (dedicated runner)
CR-7	minor	A stale "[to be validated: measured by scenarios.yml]" comment, a job named "three scenarios" that runs four, an unused loop variable	fixed in the commit that adds this page: the comment points at the measurement in dev-env §5 (run 40685163237), the job is "four scenarios in parallel", the variable is _
SEC-T5-01	major	down ran sind, kind and docker through a PATH that put .scenarios/N/tools/bin first, which cluster jobs can write through the sind data mount	fixed in the commit that adds this page: down uses the host environment plus KUBECONFIG only; unit test (subprocess mocked) that no command or lookup gets a PATH with the data directory
SEC-T5-02	major	Every kind node mounted the whole repository read-write, so a pod could read or change other scenarios' .scenarios/<id>/ (kubeconfigs, tools)	fixed in the commit that adds this page: the repository is mounted readOnly: true (kind extraMounts[].readOnly, kind configuration), plus one read-write mount of the data directory (.scenarios/N, or .scenarios/k8s for the T4 harness); parallel.sh checks with docker inspect that the only read-write bind under the repository is .scenarios/3; green in runs 40688967336 and 40688967168
SEC-T5-03	minor	.scenarios/ and the data directories are not checked against symlinks	partly fixed in the commit that adds this page: claim refuses a symlinked .scenarios/; the rest deferred
SEC-T5-04	minor	No ownership check on the ID lock or on the spx-* labels: any local Docker user can act on another user's scenario	deferred: recorded in dev-env §5 "Built in T5" and T5 Findings item 9
SEC-T5-05	minor	--id 0 bypassed the explicit-ID path (same as CR-3)	fixed with CR-3
SEC-T5-06	minor	The kind API server was published on every interface	fixed in the commit that adds this page: apiServerAddress: 127.0.0.1 next to apiServerPort
SEC-T5-07	minor	The Kueue manifest went to a shared path under .scenarios/, checked and applied there, so parallel runs could race between check and apply	fixed in the commit that adds this page: a mktemp file in the scenario's data directory, checked by SHA-256, the same file applied, then deleted
CR-8	blocker	Round 2: a failed up ran down(n), which deletes by the host-global names spx-<n>; a second checkout whose claim returned an ID held by another checkout would tear that checkout's scenario down	fixed: up first owns the network spx-<n> (docker network create refuses a duplicate name) and only then may clean up by name; before that, a failure only releases the claim; unit test
CR-9	minor	Round 2: with sind only in the data directory, down cannot remove a SLURM realm, so a failed up leaks it	fixed: up refuses before claim when the layer's sind or kind is not on the host PATH, and down's message names bash dev/bootstrap.sh
CR-10, SEC-T5-09	minor	Round 2: a corrupt scenario.json (cluster-writable) made down crash before any teardown	fixed: the parse is guarded, down removes whatever exists; unit test
CR-11	minor	Round 2: "what it created was removed" was printed even when the cleanup failed	fixed: up's cleanup prints its own outcome; main says "see the lines above"
CR-12	minor	Round 2: two Down tests leaked run() lines into the test output	fixed: sys.stderr patched
SEC-T5-08	minor	Round 2: the admin kubeconfig and scenario.json sit in the directory the kind nodes mount read-write	deferred: recorded in dev-env §5 "Not built" (a work/ subdirectory as the only read-write mount); the read-only repository mount guards against mistakes, not against a hostile hostPath pod on a privileged kind node
SEC-T5-10	minor	Round 2: the Kueue manifest was checked and applied from the read-write data directory	fixed: downloaded to the host's TMPDIR, removed on every path (a RETURN trap tried first broke pipelines-k8s kueue: bash keeps the trap after the function returns, and it fired later on an unset manifest, run 40690398142)
SEC-T5-11	minor	Round 2: parallel.sh's cleanup trap ran down for IDs 1 to 4 even when this run had not created them	fixed: the script refuses to start when any .scenarios/1..4 exists
SEC-T5-12	minor	Round 2: the remaining symlink exposure (SEC-T5-03) was not in the owning spec	fixed: dev-env §5 "Not built" names it
CR-13	major	Round 3: parallel.sh's cleanup trap ran down for every ID after a failed up, so an up --id 3 refused because another checkout holds spx-3 still led to that checkout's scenario being torn down	fixed in PR #43 (aaff4e0): the trap runs down only for IDs whose .scenarios/<id>/ this run still holds
CR-14	minor	Round 3: down removed the network, the ID's ownership token, even after an earlier step failed, so another checkout could reserve the ID while this one still had a half-removed scenario	fixed in PR #43 (aaff4e0): the network is released last and only when every other step succeeded; unit test
CR-15	minor	Round 4: the header of parallel.sh still said the failure trap runs down for every ID	fixed: the header names only the IDs whose .scenarios/<id>/ this run still holds
CR-16	minor	Round 4: no test runs the CR-13 path (a released ID skipped by the trap); CI follows the success path only	accepted: checked by reading the code only, recorded on the Progress page (finding 11)