| 1 | # Review brief: scanner host rollout spec |
| 2 | |
| 3 | You are reviewing an infrastructure and rollout spec independently of its |
| 4 | author. Your output is a findings report, not an edit. Do not modify any file. |
| 5 | |
| 6 | ## What to read |
| 7 | |
| 8 | 1. The spec under review: |
| 9 | `docs/superpowers/specs/2026-10-04-scanner-host-rollout-design.md`. |
| 10 | 2. Its parents: `docs/superpowers/specs/2026-09-01-object-storage-virus-scanning-design.md` |
| 11 | (§4, §5, §10) and `docs/superpowers/specs/2026-09-02-scan-on-demand-design.md` §9, |
| 12 | and the rollout plan `docs/superpowers/plans/2026-09-01-carloku-scanning-rollout.md`. |
| 13 | 3. The code it relies on, in this worktree: `cmd/console/main.go` |
| 14 | (`storeScannerFor`, `scannerWiring`, `storeBackend`), `internal/clamd/clamd.go`, |
| 15 | `internal/objstore/clamav.go`, `scanner.go`, `sweep.go`, `scantag.go`, |
| 16 | `internal/console/scanpass.go`, `internal/console/apisystem.go`, `system.go`, |
| 17 | `infra/modules/region-host/main.tf`, `variables.tf`, `outputs.tf`. |
| 18 | 4. Debian trixie's `clamav-daemon` and `clamav-freshclam` packaging behaviour |
| 19 | (socket activation, `clamd.conf` keys `TCPSocket`, `TCPAddr`, |
| 20 | `StreamMaxLength`, `MaxScanSize`, `MaxFileSize`, `ConcurrentDatabaseReload`) |
| 21 | as you know it. |
| 22 | |
| 23 | ## Fixed decisions (do not re-argue; report only a mechanism that fails one) |
| 24 | |
| 25 | D1 to D7 in spec §2. In particular D3: plain TCP inside the VPC, fenced by a |
| 26 | single-source security group, was chosen by the owner. D7: scope ends at |
| 27 | "engine wired, switched on, no member granted". |
| 28 | |
| 29 | ## What matters most |
| 30 | |
| 31 | - **Does the console actually work with what §4 and §6 supply?** Check every |
| 32 | S3 action the scanner, sweep, tagging, enforce and teardown paths call |
| 33 | against the §4.1 grant and the existing `host_store` policy. A missing action |
| 34 | shows up as `denied` counts or a failed pass in production; name it. |
| 35 | - **Boot refusals.** Every environment variable the console requires once a |
| 36 | clamd address is set, and whether §6.2 supplies them all. Anything that would |
| 37 | crash-loop the console (the 2026-09-11 Tito outage was exactly this). |
| 38 | - **The module.** Can the security group, VPC/subnet preconditions, IMDSv2, |
| 39 | `ignore_changes`, and the user_data sequence (freshclam before clamd, masking |
| 40 | the socket unit, binding the private IP) actually produce a clamd answering on |
| 41 | `<private_ip>:3310` and nowhere else? Anything that leaves clamd on 0.0.0.0, |
| 42 | or a box that reports healthy while clamd is not listening on TCP. |
| 43 | - **StreamMaxLength vs CARLOS_CLAMD_MAX_BYTES.** Is §3.2's rule right for how |
| 44 | `internal/clamd` and `objstore/clamav.go` decide to skip an over-size object? |
| 45 | Any off-by-one or unit mismatch (MB vs MiB, bytes vs "25M"). |
| 46 | - **The proof in §7.** Does each step test what it claims? Would EICAR be |
| 47 | reported as infected through this code path? Is step 3's unwire/rewire safe |
| 48 | given how `system/antivirus.json` and `enforce` behave? |
| 49 | - **The rollout mechanics in §5 and §6.** Targeted applies, the partial ref |
| 50 | bump, key generation and storage. Anything that loses the key, leaks it, or |
| 51 | restarts a box unintentionally. |
| 52 | |
| 53 | ## Output format |
| 54 | |
| 55 | Number each finding. For each: severity (P1 blocks rollout, P2 should fix |
| 56 | before rollout, P3 nice to have), the spec section, the code citation |
| 57 | (file:line) that proves it, what goes wrong concretely, and a suggested fix. |
| 58 | End with a one-line verdict: accept, accept with fixes, or reject. |
| 59 | |
amadan