boma/docs/reviews/2026-05-30-review.md

# Repo review — 2026-05-30

- **Reviewed commit:** `de38d1c`
- **Mode:** on-demand (interactive); safe fixes applied, rest reported.
- **Method:** `scripts/repo-scan.py` (Phase 0) + 3 fan-out reviewers (consistency,
  drift/cruft, conformance) + cross-cutting synthesis.

## Summary

| | high | medium | low | total |
|---|---|---|---|---|
| Auto-fixed | 2 | 3 | 2 | 7 |
| Open (reported) | 4 | 4 | 9 | 17 |

Dominant theme: **drift created by this session's own rapid changes** — residual
`.vault_pass` references after the Vaultwarden/rbw switch, and leftover
PR/merge-request language after going trunk-based.

## Auto-fixes applied (safe + obvious)

| File | Fix |
|---|---|
| `README.md` | `terraform/` tree comment: "infra DNS" → "no DNS" |
| `docs/decisions/003-toolchain.md` | closing line: "and infrastructure DNS" → "only (no DNS)" |
| `STATUS.md` | hosts.yml row: "commented examples" → "structured stubs with empty host maps" |
| `CLAUDE.md` | don't-modify bullet generalised `production` → `<env>` |
| `docs/runbooks/new-role.md` | "open PR / merge request" → "merge to main once make test passes" |
| `CONTRIBUTING.md` | "Before opening a merge request" → "Before committing or merging to main" |
| `scripts/README.md` | documented the 3 undocumented scripts |

Also hardened `scripts/repo-scan.py` itself: the marker check no longer matches
filenames like `TODO.md`, and path-ref checks only fire inside backticks/links (no
prose false positives).

## Open findings

### Secrets — residual `.vault_pass` references (reported, not auto-fixed: secrets-adjacent)
- **[high] R1** `docs/decisions/005-bootstrapping.md:64-65` — control-node setup still
  says `cp /secure/location/.vault_pass …`. Replace with the rbw setup (ADR-002,
  rotate-secrets runbook).
- **[high] R2** `docs/runbooks/new-host.md:6` — prerequisite "you have the vault
  password (`.vault_pass`)". → "rbw installed and unlocked".
- **[high] R3** `docs/runbooks/new-host.md:129-130` — "place `.vault_pass`" step →
  rbw unlock.
- **[medium] R4** `CONTRIBUTING.md:32-33` — secrets section describes `.vault_pass`
  secure-channel sharing → point to rbw/Vaultwarden.
- **[medium] R5** `AGENTS.md:16` — "Never read/print/commit `.vault_pass`" names a
  file that no longer exists → reword to the rbw flow.

### Workflow model — trunk-based vs PR/approval-gates (judgement)
- **[high] R6** `docs/decisions/003-toolchain.md` (CI/CD) & `docs/decisions/008-testing.md`
  (CI pipeline) describe pull-requests-to-main + manual approval gates, contradicting
  the trunk-based, no-MR model in CLAUDE.md/CONTRIBUTING. Decide: keep *deploy*
  approval gates (reasonable even solo) but drop the PR framing, or fully reconcile.
- **[medium] R7** `CLAUDE.md:151` — "Push to `main` branch" listed as forbidden,
  contradicting the trunk-based "commit straight to main" policy a few lines above.

### Roles not in git (judgement)
- **[high] R8** `roles/` — `base` and `docker_host` are empty dirs on disk and **not
  tracked by git** (empty dirs aren't committed). A fresh clone has neither, so
  `make deploy PLAYBOOK=site` would **error** (role-not-found), not "apply nothing"
  as STATUS.md:29-30 says. Decide: scaffold them (`make new-role`) or correct the docs.

### Terraform correctness
- **[medium] R9** module `vlan_tag` is never passed by either environment → VMs land
  on untagged `vmbr0`, contradicting ADR-007's srv VLAN. Wire `vlan_tag` through.
- **[low] R10** `<owner>/<repo>` placeholders unresolved in `backend.tf` (×2),
  Makefile `MOLECULE_IMAGE`, `.scaffold/molecule.yml` — non-functional as committed.

### Scaffolding (new-role)
- **[medium] R11** `make new-role` copies `.scaffold/converge.yml` with literal
  `ROLE_NAME_PLACEHOLDER`, never substituted → scaffolded molecule references a
  non-existent role. Sed-substitute `NAME` or document the manual step.
- **[low] R12** `Makefile:154` writes the role README via `echo "…\n…"`; `\n` isn't
  expanded in `sh` → literal `\n`. Use `printf`.

### Other consistency / drift
- **[low] R13** `CLAUDE.md:93` — group_vars tree lists `control/`, which doesn't exist
  on disk. Create it or drop the line.
- **[low] R14** `docs/decisions/006-terraform.md:40` — "No Galaxy roles." reused in the
  Terraform section (an Ansible-collections concept). Reword.
- **[low] R15** `docs/decisions/001-architecture.md:22` — DNS row reads as present-tense
  reality though the `dns` role is unbuilt (per STATUS.md). Add a "planned" pointer.
- **[low] R16** `docs/decisions/001-architecture.md:42` — "monitoring agent" in the
  Proxmox baseline isn't reflected in ADR-002's baseline. Align.
- **[low] R17** `playbooks/bootstrap.yml:12-16` — raw python-install task hardcodes
  `changed_when: false` → never reports a change. Consider an honest `changed_when`.

## Suggested follow-up prompt

> Remediate the boma 2026-05-30 review (`docs/reviews/2026-05-30-review.md`):
> 1. Purge the residual `.vault_pass` references R1–R5 → the rbw/Vaultwarden flow.
> 2. Decide the workflow model R6–R7 — I lean "keep deploy approval gates, drop the
>    PR/merge-request framing"; reconcile ADR-003/008 and CLAUDE.md to match.
> 3. Resolve R8 — scaffold `base`/`docker_host` via `make new-role`, or correct
>    STATUS.md/roles/README.md to say the roles don't exist yet.
> 4. Fix the Terraform `vlan_tag` wiring (R9).
> Report on the rest.