Skip to content

Code review · D3 · Pinned toolchain, bootstrap and doctor

Range: 98d74c2..51c759a on autopilot/D3. Test command: bash dev/doctor.sh (exit 0, doctor: all layers passed (2 skipped) in a cloud session), plus shellcheck -x dev/bootstrap.sh dev/doctor.sh dev/lib.sh (clean) and bash dev/bootstrap.sh --ci (bootstrap: done). Reviewers ran as independent headless sessions (claude -p --agent …, read-only tools).

Pass Round 1 (HEAD 11171d0) Round 2 (HEAD e7f5dc6)
code-reviewer approve, b/M/m = 0/0/4 approve, 0/0/1
security-reviewer changes, 0/1/3 approve, 0/0/1
code-simplifier (HEAD 51c759a) — changed, -1/+0, tests pass

The round-2 minors were fixed in 0729ab2, and the simplifier ran on that commit.

CR-01 · minor · The §11 Nextflow version and the required check names disagree with the new pin

  • Where: dev-environment-and-testing.md §11, pipelines-synthetic.yml row; docs/playbooks/autopilot.md:220
  • Evidence: dev/versions.env pins NEXTFLOW_VERSION=26.10.4; §11 and the required status check names still say 26.10.3.
  • Problem: two values for one pin.
  • Proposal: point §11 at the pins, or keep 26.10.3; put the check names to Thomas, since autopilot.md is outside this step.
  • Found by: code-reviewer
  • Status: applied (§11). The §11 row says its 26.10.3 matrix predates versions.env. Aligning the matrix and the check names is inbox #42.

CR-02 · minor · The decision log links to a Progress page that did not exist yet

  • Where: docs/decisions/log.md, D3 entry
  • Evidence: ../progress/W3/D3.md was missing at review time.
  • Problem: mkdocs build --strict would fail.
  • Proposal: write the page in the step's docs pass.
  • Found by: code-reviewer
  • Status: applied (Progress page).

CR-03 · minor · The doctor did not check the Kubernetes executor plugin

  • Where: dev/doctor.sh, versions block
  • Evidence: bootstrap installs snakemake-executor-plugin-kubernetes, but only the SLURM plugin's version was compared.
  • Problem: drift in that plugin would go unnoticed.
  • Proposal: add a smk-kubernetes version line.
  • Found by: code-reviewer
  • Status: applied (e7f5dc6).

CR-04 · minor · Dependency trees were not locked

  • Where: dev/bootstrap.sh, uv tool install and npm install
  • Evidence: only the top-level packages had exact pins.
  • Problem: two cold runs could install different code.
  • Proposal: note the gap in §1, or lock.
  • Found by: code-reviewer
  • Status: applied with SEC-D3-01 (both trees are now hash-locked).

CR-05 · minor · The §1 table still said uv tool install for Snakemake

  • Where: dev-environment-and-testing.md §1, Snakemake row
  • Evidence: the "Bootstrap" paragraph describes the locked venv, but the table cell did not.
  • Problem: the spec described one install in two ways.
  • Proposal: name dev/snakemake.lock.txt and uv pip sync --require-hashes in the cell.
  • Found by: code-reviewer
  • Status: applied (0729ab2).

SEC-D3-01 · major · PyPI and npm dependency trees were neither locked nor hash-checked

  • Where: dev/bootstrap.sh (Snakemake and Playwright installs); the spec claim "every download is checked"
  • Evidence: dozens of transitive packages were resolved fresh on each cold run, as root, in the cloud setup script and in CI.
  • Problem: a compromised dependency would run as root with repository credentials nearby, and the spec stated a control the code did not have.
  • Proposal: a hash-locked requirements file installed with --require-hashes, a package-lock.json installed with npm ci, and a spec naming the remaining unhashed downloads.
  • Found by: security-reviewer
  • Status: applied (e7f5dc6). dev/snakemake.lock.txt (72 packages, every one with --hash=sha256:) goes into a venv named after the lock with uv pip sync --require-hashes; dev/npm/package-lock.json goes in with npm ci. A tampered hash was refused (Hash mismatch, exit 1). §1 "Checked downloads" names the two downloads not hashed by us: the uv-managed CPython [to be validated] and Playwright's Chromium.

SEC-D3-02 · minor · Running as root, tar kept the archive's owners

  • Where: dev/bootstrap.sh, install_tar
  • Evidence: GNU tar keeps stored uids and gids by default when run as root.
  • Problem: on a shared host, a matching uid could replace node or helm.
  • Proposal: --no-same-owner --no-same-permissions.
  • Found by: security-reviewer
  • Status: applied (e7f5dc6).

SEC-D3-03 · minor · The doctor runs images by tag after checking them by digest

  • Where: dev/doctor.sh (hello-world, sind config, kind --image, the Job image)
  • Evidence: ensure_image checks the digest, then the tools start the image by name:tag.
  • Problem: something could retag the image between the check and the run.
  • Proposal: run by digest where the tool accepts it.
  • Found by: security-reviewer
  • Status: rejected (the reviewer accepted the reason in round 2). An image pulled from mirror.gcr.io carries only a mirror reference, so name@sha256:… would bypass the mirror and pull from Docker Hub, which rate-limits (spike D2). Retagging needs control of the Docker daemon, which is already root.

SEC-D3-04 · minor · The cloud allowlist has wildcards anyone can host under

  • Where: dev-environment-and-testing.md §2, Custom network list (*.quay.io, *.pkg.dev, *.prefix.dev)
  • Evidence: any Google Cloud project can publish under *.pkg.dev.
  • Problem: wider egress than bootstrap needs.
  • Proposal: replace each wildcard by the exact hosts seen at the D2 re-probe.
  • Found by: security-reviewer
  • Status: accepted. It is done at the D2 re-probe (follow-up 3); §2 says so next to the list.

SEC-D3-05 · minor · A refused npm ci was reported as success on the next run

  • Where: dev/bootstrap.sh, install_node_and_playwright
  • Evidence: the lock was copied before npm ci ran, so after a failure the next cmp matched and the install was skipped.
  • Problem: a green bootstrap over a refused install (the doctor still failed).
  • Proposal: write a marker only after npm ci succeeds; add --ignore-scripts.
  • Found by: security-reviewer
  • Status: applied (0729ab2: .installed-lock is written after success; --ignore-scripts).

Simplifier

51c759a removed export UV_TOOL_BIN_DIR, left without a reader once round 1 replaced uv tool install with the locked venv. It left the three install_bin nextflow lines as they are (a loop would need indirect expansion). shellcheck, bootstrap and doctor re-run: pass.

CODE  step=D3  review=approve b/M/m=0/0/5  security=approve b/M/m=0/1/4  rounds=2  simplify=changed -1/+0  tests=pass