| 1 | # Review brief: scanner host, the code |
| 2 | |
| 3 | You are reviewing the implementation of an infrastructure spec you already |
| 4 | reviewed six times as a design. Your output is a findings report, not an edit. |
| 5 | Do not modify any file. |
| 6 | |
| 7 | ## What to read |
| 8 | |
| 9 | 1. The spec: `docs/superpowers/specs/2026-10-04-scanner-host-rollout-design.md` |
| 10 | (draft 6). Decisions D1-D8 in §2 are fixed owner rulings: report a mechanism |
| 11 | that fails one, do not re-argue one. Your earlier design findings are in |
| 12 | `CODEX-FINDINGS-SCANNER-HOST*.md` beside it. |
| 13 | 2. The plan: `docs/superpowers/plans/2026-10-05-scanner-host-rollout.md`. |
| 14 | Tasks 1-4 are what was built. Tasks 7 and 9-10 show how the module and its |
| 15 | outputs are used on real boxes. |
| 16 | 3. The code, as the branch's own changes against main. The merge-base is the |
| 17 | commit the branch started from; review `git diff <merge-base>..HEAD` for: |
| 18 | - `internal/clamd/clamd.go`, `clamd_test.go` |
| 19 | - `infra/modules/region-host/{main,variables}.tf`, `tests/store_scanning.tftest.hcl` |
| 20 | - `infra/modules/scanner-host/` (all of it: `main.tf`, `variables.tf`, |
| 21 | `outputs.tf`, `scanner-init.sh.tftpl`, `files/*`, `tests/*`, `README.md`) |
| 22 | - `docs/site/cli.md`, `infra/modules/README.md`, `infra/modules/region-host/README.md` |
| 23 | Ignore everything that arrived from main through the merge commit. |
| 24 | 4. Debian trixie's `clamav` 1.4.3 packaging and ClamAV 1.4.3 sources, as you |
| 25 | know them or can fetch them. Earlier rounds relied on `libclamav/unzip.c`, |
| 26 | `others.c`, `debian/usr.sbin.clamd`, `clamav-daemon.config.in`, |
| 27 | `clamav-daemon.postinst.in`, `clamav-daemon.service.in`. |
| 28 | |
| 29 | ## What matters most |
| 30 | |
| 31 | - **The invariant** (spec §3.2): TCP 3310 is open only while a self-test has |
| 32 | passed against the clamd process running now. Trace the three units and the |
| 33 | clamd drop-in through: first boot, reboot, clamd crash and auto-restart, |
| 34 | explicit restart, a package upgrade (which now runs with |
| 35 | `clamav-daemon/debconf` false), a freshclam signature reload, a failed |
| 36 | self-test, and a direct restart of the self-test. Name any state in which the |
| 37 | port is open without a passed test, or never reopens after one. |
| 38 | - **The first-boot script** (`scanner-init.sh.tftpl`): every failure path must |
| 39 | print `SCANNER-HOST FAILED: <reason>` and exit non-zero, and READY must never |
| 40 | print with the invariant unmet. Check `set -euo pipefail` and the ERR trap |
| 41 | (not inherited by functions), template escaping (`${...}` is templatefile), |
| 42 | the AppArmor local include and reload, the debconf freeze and its position |
| 43 | relative to the package's own `clamd.conf` generation, and the copied |
| 44 | `ExecStartPre` lines. |
| 45 | - **The self-test**: can it pass while the property it guards fails? |
| 46 | - **The Go change**: every consumer of `clamd.Status` after the change. |
| 47 | - **IAM**: least privilege on both roles; the scanner role's Deny. |
| 48 | - **Tests**: does each `tofu test` run and Go test pin what it claims? |
| 49 | - **The docs**: anything an operator following the README or `cli.md` would |
| 50 | get wrong because the docs and the code disagree. |
| 51 | - **The acceptance checks** in plan Task 7: could any pass on a box that is |
| 52 | actually wrong? |
| 53 | |
| 54 | ## Output format |
| 55 | |
| 56 | Number findings from 13 (1-12 were the design rounds). For each: severity |
| 57 | (P1 blocks applying this to a live deployment, P2 should fix first, P3 nice to |
| 58 | have), file:line, what goes wrong concretely, and a fix. End with one line: |
| 59 | accept, accept with fixes, or reject. |
| 60 | |
amadan