212 lines
14 KiB
Markdown
212 lines
14 KiB
Markdown
# Cross-Code Review Report — Job 1ed5cf56
|
|
|
|
- **Job ID**: `1ed5cf56`
|
|
- **Reviewer**: cline
|
|
- **Date**: 2026-08-23
|
|
- **Scope**: Comprehensive code and security review of commit `b09d420` (`git diff main..refactor`) — Track 1R Docker deployment assets, test guards D-22~D-30, and documentation parity.
|
|
- **Changeset**: 1 commit, 10 files changed (1524 insertions, 51 deletions)
|
|
|
|
---
|
|
|
|
## 1. Executive Summary
|
|
|
|
This review independently evaluates the production Docker deployment assets for the MAM `nats-server` remote broker, the D-22~D-30 regression guards, documentation parity between `PRIVATE_SERVER.md` / `docker/README.md` / `implementation_plan.md`, and the full test suite. The changeset is well-structured, security-conscious, and fully tested. No blocking defects, security vulnerabilities, or correctness issues were found. Two Medium-severity documentation consistency findings and two Low-severity documentation issues are reported as actionable improvements.
|
|
|
|
**[VERDICT: PASS]**
|
|
|
|
---
|
|
|
|
## 2. Changeset Overview
|
|
|
|
| File | Change |
|
|
|---|---|
|
|
| `docker/docker-compose.yaml` | **New** (40 lines) — Production compose for `nats:2.12-alpine` |
|
|
| `docker/nats.conf` | **New** (75 lines) — NATS server config (JetStream, MQTT, WebSocket, accounts) |
|
|
| `docker/.env.example` | **New** (47 lines) — Secret template (empty values, fail-closed) |
|
|
| `docker/README.md` | **New** (158 lines) — Deployment guide + verification playbook |
|
|
| `tests/test_deploy_freshness.py` | **Modified** (+234 lines) — D-22~D-30 guards + D-16 tightening |
|
|
| `PRIVATE_SERVER.md` | **Modified** (+129/-...) — §5.1/§5.2 transport paths, §9 canonical-asset note, §9.4 R-table |
|
|
| `implementation_plan.md` | **Modified** (+21/-...) — P0.5 step, M2b checklist updates |
|
|
| `requirements.txt` | **New** (2 lines) — `pytest>=8.0`, `PyYAML>=6.0` |
|
|
|
|
---
|
|
|
|
## 3. Review Area 1 — Docker Assets
|
|
|
|
### 3.1 Security ✅
|
|
|
|
| Check | Result |
|
|
|---|---|
|
|
| Fail-closed secrets (compose `${VAR:?error}`) | ✅ All 4 secrets use `${VAR:?set ...}` — empty `.env` aborts container creation |
|
|
| No literal secrets in `nats.conf` | ✅ All `password:` values are `$VAR` references; regex `password:\s*([^\s,}]+)` confirms no plaintext |
|
|
| `.gitignore` excludes `docker/.env`, tracks `.env.example` | ✅ D-29 verifies: `git check-ignore docker/.env` → ignored; `.env.example` → not ignored; `git ls-files docker/.env` → empty |
|
|
| Loopback binding by default | ✅ `MQTT_BIND`, `NATS_BIND`, `WS_BIND` default to `127.0.0.1` via `${VAR:-127.0.0.1}` |
|
|
| 8222 monitoring port hardcoded loopback | ✅ `127.0.0.1:8222:8222` — no variable override (unauthenticated endpoint) |
|
|
| Observer write-protected | ✅ `publish: { deny: [">"] }` — observer cannot publish any subject |
|
|
| Account isolation | ✅ `MAM`, `HOME`, `SYS` are separate NATS accounts; cross-account subjects invisible |
|
|
| UFW bypass documented | ✅ Both `docker/README.md` §4 and `PRIVATE_SERVER.md` §9.2 WARNING explain Docker port-bypass |
|
|
|
|
### 3.2 Correctness ✅
|
|
|
|
| Check | Result |
|
|
|---|---|
|
|
| Image pinned `nats:2.12-alpine` (not `latest`) | ✅ D-23 enforces; tag found in PRIVATE_SERVER.md |
|
|
| Healthcheck uses alpine `wget` → `/healthz` on `127.0.0.1:8222` | ✅ D-28 verifies `wget`, `/healthz`, `127.0.0.1:8222`, `"alpine" in image` |
|
|
| JetStream `store_dir: "/data"` matches volume `/data` | ✅ D-26 verifies both sides |
|
|
| `max_file: 10G`, `max_mem: 256M` (uppercase suffix) | ✅ D-26 regex `^\d+[KMGT]$` |
|
|
| MQTT port 1883, `ack_wait: 60s`, `max_ack_pending: 1024` | ✅ |
|
|
| WebSocket `no_tls: true` (required for startup) | ✅ D-30 enforces presence in active config |
|
|
| `system_account: SYS` | ✅ |
|
|
| MAM account `jetstream: enabled` (required for MQTT retained) | ✅ D-26 verifies |
|
|
| `/mqtt` WebSocket path documented (N-7) | ✅ D-30 verifies `/mqtt` in conf |
|
|
| `allowed_origins` never `"*"` (NATS rejects it) | ✅ D-30 scans active lines |
|
|
|
|
### 3.3 Byte-Level Parity (docker/ ↔ PRIVATE_SERVER.md) ✅
|
|
|
|
Automated comparison confirms exact byte-for-byte match (after strip) between:
|
|
- `docker/nats.conf` ↔ `PRIVATE_SERVER.md` §9.1 code fence → **MATCH**
|
|
- `docker/docker-compose.yaml` ↔ `PRIVATE_SERVER.md` §9.2 code fence → **MATCH**
|
|
|
|
---
|
|
|
|
## 4. Review Area 2 — Test Coverage (D-22 ~ D-30)
|
|
|
|
Nine new guards protect the docker/ canonical assets against silent drift. Each was reviewed for correctness, mutation-sensitivity, and false-positive risk.
|
|
|
|
| Guard | Purpose | Assessment |
|
|
|---|---|---|
|
|
| **D-22** | All 4 docker/ assets exist and are non-empty; compose parses as YAML with a `nats` service | ✅ Catches accidentally-empty or unpopulated assets |
|
|
| **D-23** | Compose image tag ≠ `latest`, contains `alpine`, and the tag appears in PRIVATE_SERVER.md | ✅ Cross-doc coupling; catches pin drift |
|
|
| **D-24** | Exactly 4 container ports (1883, 4222, 8222, 8080); all mappings are 3-part; 8222 hardcoded to `127.0.0.1:8222:8222` | ✅ Prevents bare port mappings and monitoring port exposure |
|
|
| **D-25** | `$VAR` refs in active nats.conf ⊆ compose environment keys; all env values use `${VAR:?error}`; `.env.example` secrets are empty; no literal passwords in conf | ✅ Comprehensive fail-closed enforcement; catches placeholder secrets |
|
|
| **D-26** | Volume mounts to `/data`; `store_dir: "/data"`; `max_file`/`max_mem` match `^\d+[KMGT]$`; `mqtt {` block with `port: 1883`; `MAM:` account has `jetstream: enabled` | ✅ JetStream/MQTT contract integrity |
|
|
| **D-27** | Job subjects in nats.conf start with `python.mqtt.jobs.` (= `DEFAULT_TOPIC_ROOT` dotted); `mam_observer` present with `deny:` | ✅ Observer permissions track topic root; catches topic-root migration drift |
|
|
| **D-28** | Healthcheck uses `wget`, targets `/healthz` at `127.0.0.1:8222`; image contains `alpine` | ✅ Healthcheck/image coupling; catches non-alpine image swap |
|
|
| **D-29** | `docker/.env` git-ignored; `docker/.env.example` NOT ignored; `docker/.env` never tracked | ✅ Secret hygiene via `.gitignore` (lines 21-23: `.env` / `.env.*` / `!.env.example`) |
|
|
| **D-30** | Active conf has `websocket {` + `no_tls: true`; no `"*"` in `allowed_origins`; `/mqtt` path documented | ✅ WebSocket startup safety; catches star-origin and missing MQTT path |
|
|
|
|
### D-16 Guard Tightening
|
|
|
|
D-16 (`test_d16_private_server_nats_image_alpine_pinned`) was tightened from the previous looser check to `assert "alpine" in tag`. This is correct: the healthcheck uses `wget` which only exists in the `alpine` variant, so any non-alpine image would produce a permanently `unhealthy` container. The tighter assertion closes the gap where a tag like `nats:2.12-scratch` would have passed.
|
|
|
|
### Coverage Assessment
|
|
|
|
The D-22~D-30 suite provides **comprehensive regression protection** for the docker/ assets. Key strengths:
|
|
- **Cross-document coupling** (D-23, D-27) ties compose/conf to PRIVATE_SERVER.md and `mqtt_common.DEFAULT_TOPIC_ROOT`, preventing silent drift.
|
|
- **Active-line filtering** (`_active_conf_lines()`) strips comments before assertion, preventing false passes from commented-out templates.
|
|
- **Defense-in-depth** — fail-closed (D-25), port exposure (D-24), healthcheck coupling (D-28), and secret hygiene (D-29) are independently guarded.
|
|
|
|
---
|
|
|
|
## 5. Review Area 3 — Documentation Parity
|
|
|
|
### 5.1 Byte-Level Parity (docker/ ↔ PRIVATE_SERVER.md) ✅
|
|
|
|
As confirmed in §3.3, the `nats.conf` and `docker-compose.yaml` code fences in `PRIVATE_SERVER.md` §9.1/§9.2 are byte-for-byte identical to the canonical `docker/` files. The §9 NOTE correctly declares `docker/` as canonical and warns that D-22~D-30 guards enforce parity.
|
|
|
|
### 5.2 R-ID Collision (PRIVATE_SERVER.md §9.4 vs docker/README.md §7) — M-1
|
|
|
|
Both documents define an "R-1 ~ R-10" verification playbook table, but **6 of 10 R-IDs have different meanings**:
|
|
|
|
| R-ID | PRIVATE_SERVER.md §9.4 | docker/README.md §7 | Match? |
|
|
|---|---|---|---|
|
|
| R-1 | Broker health (curl /healthz) | Health endpoint (curl /healthz) | ✅ Same |
|
|
| R-2 | Listener + TLS identity (varz, SAN) | External monitoring blocked (curl public IP) | ❌ **Different** |
|
|
| R-3 | Port exposure assertion (nmap) | Port exposure (nmap) | ✅ Same |
|
|
| R-4 | Round-trip pub/sub + JetStream | WAN latency (`python latency_check.py`) | ❌ **Different** |
|
|
| R-5 | Retained terminal event (MQTT) | Auth rejection (Not authorized) | ❌ **Different** |
|
|
| R-6 | Broker identity assertion (no hivemq) | Auth success (rc=0) | ❌ **Different** |
|
|
| R-7 | Freeze regression (H-1/H-4) | Retained event delivery | ❌ **Different** |
|
|
| R-8 | Full regression suite (pytest) | Broker identifier (mam-hub) | ❌ **Different** |
|
|
| R-9 | Observer account boundary | Tenant account isolation | ✅ Same |
|
|
| R-10 | Retained boundary (N-1) | Retained boundary (N-1/N-7) | ✅ Same |
|
|
|
|
**Impact**: An operator cross-referencing "R-5" between the two documents would execute the wrong test. For example, README's R-7 (retained delivery) ≈ PRIVATE_SERVER's R-5 (retained terminal event) — same concept, different number.
|
|
|
|
**Recommendation**: Re-number the README table (e.g., `RD-1`~`RD-10` or a distinct prefix) or align both tables to a single canonical definition in PRIVATE_SERVER.md and have README reference it.
|
|
|
|
### 5.3 Undefined R-11/R-12/R-13 — M-2
|
|
|
|
`implementation_plan.md` references R-11~R-13 (specifically R-13 as a "final gate"), but neither `PRIVATE_SERVER.md` §9.4 nor `docker/README.md` §7 defines them:
|
|
|
|
- `implementation_plan.md:122` — `R-1 ~ R-13. R-5(retained) / R-9(계정 경계) / R-13(MQTT-over-WS)`
|
|
- `implementation_plan.md:182` — `R-1 ~ R-13 전건 통과 (R-5 / R-9 / R-13 최종 관문)`
|
|
|
|
R-13 is described as "MQTT-over-WS" verification (the `/mqtt` WebSocket path from N-7), which is operationally critical, but no command/pass-criteria row exists for it in either document.
|
|
|
|
**Recommendation**: Add R-11, R-12, R-13 rows to the PRIVATE_SERVER.md §9.4 table (canonical source) and update docker/README.md §7 to reference rather than duplicate.
|
|
|
|
### 5.4 `latency_check.py` Reference — L-1
|
|
|
|
`docker/README.md` §7 R-4 instructs `python latency_check.py` (expected: RTT P95 < 150ms), but **no `latency_check.py` file exists** anywhere in the repository (`find` confirms zero results). The canonical latency probe lives as an inline Python heredoc in `PRIVATE_SERVER.md` §9.4 (below the R-table). This is compounded by the R-4 collision (§5.2): README's R-4 is WAN latency, PRIVATE_SERVER's R-4 is pub/sub+JetStream.
|
|
|
|
**Recommendation**: Either ship a `docker/latency_check.py` script or replace the README reference with the inline heredoc from PRIVATE_SERVER.md.
|
|
|
|
### 5.5 UFW Rules Divergence — L-2
|
|
|
|
| Rule | docker/README.md §4 | PRIVATE_SERVER.md §9.3 |
|
|
|---|---|---|
|
|
| Tailnet allow | `sudo ufw allow in on tailscale0 to any` (all ports) | `sudo ufw allow in on tailscale0 to any port 1883/4222/8080 proto tcp` (granular) |
|
|
| SSH | `sudo ufw allow ssh` | `sudo ufw allow 22/tcp` |
|
|
|
|
The README's broader `allow in on tailscale0 to any` is more permissive than PRIVATE_SERVER.md's granular per-port rules. Since Docker bypasses UFW (documented in both), UFW is secondary defense — but the README's broader rule weakens defense-in-depth on the tailnet interface.
|
|
|
|
**Recommendation**: Align README §4 UFW rules with PRIVATE_SERVER.md §9.3 granular per-port rules.
|
|
|
|
### 5.6 implementation_plan.md P0.5 Diagram Alignment — V-1
|
|
|
|
The new P0.5 step was inserted into the §5 roadmap diagram. The `▼` markers and label spacing were adjusted, but content columns are not perfectly aligned across all lines (Korean double-width characters cause visual offset). Purely cosmetic; no functional impact.
|
|
|
|
---
|
|
|
|
## 6. Review Area 4 — Full Test Suite
|
|
|
|
Command: `.venv/bin/python -m pytest tests/ -q`
|
|
|
|
```
|
|
306 passed in 353.84s (0:05:53)
|
|
```
|
|
|
|
| Metric | Value |
|
|
|---|---|
|
|
| Total tests collected | 306 |
|
|
| Passed | 306 |
|
|
| Failed | 0 |
|
|
| Errors | 0 |
|
|
| Skipped | 0 |
|
|
| Duration | 353.84s |
|
|
|
|
**Result**: 100% pass rate, 0 regressions. The full suite includes all unit tests, the 29 deploy-freshness guards (D-1~D-30), 5 tier3 integration tests, and 5 tier4 e2e tests. All green.
|
|
|
|
Subset verification (fast path, 29s): `tests/test_deploy_freshness.py + tests/test_sanity.py + tests/test_tier1_unit.py` → **76 passed in 29.14s**.
|
|
|
|
---
|
|
|
|
## 7. Findings Summary
|
|
|
|
| ID | Severity | Area | Description | Actionable? |
|
|
|---|---|---|---|---|
|
|
| M-1 | Medium | Docs parity | R-1~R-10 ID collision: 6/10 R-IDs have different meanings between PRIVATE_SERVER.md §9.4 and docker/README.md §7 | Yes — re-number or canonicalize |
|
|
| M-2 | Medium | Docs parity | R-11, R-12, R-13 referenced in implementation_plan.md but undefined in both R-tables; R-13 is a "final gate" | Yes — add rows to §9.4 |
|
|
| L-1 | Low | Docs | docker/README.md §7 R-4 references non-existent `latency_check.py` | Yes — ship script or use inline heredoc |
|
|
| L-2 | Low | Docs | UFW rules in README §4 more permissive than PRIVATE_SERVER.md §9.3 | Yes — align to granular rules |
|
|
| V-1 | Very Low | Cosmetic | P0.5 diagram label alignment inconsistent in implementation_plan.md | Optional |
|
|
|
|
### Positive Highlights
|
|
|
|
- **Fail-closed by design**: Empty `.env` aborts `docker compose up` before container creation. No placeholder secrets accepted.
|
|
- **No secrets in source**: `nats.conf` contains only `$VAR` references; `password:` regex scan confirms zero literals.
|
|
- **Loopback-first**: All variable-controlled ports default to `127.0.0.1`; 8222 is hardcoded loopback (unauthenticated monitoring).
|
|
- **Healthcheck/image coupling**: D-28 enforces that the alpine image (providing `wget`) matches the healthcheck command — a subtle but critical invariant.
|
|
- **D-16 tightening**: `assert "alpine" in tag` correctly prevents non-alpine images that would silently break the healthcheck.
|
|
- **Byte-level parity**: docker/ canonical files are exact copies of PRIVATE_SERVER.md code fences; D-22~D-30 guards enforce this automatically.
|
|
- **Active-line filtering**: `_active_conf_lines()` strips comments before assertions, preventing commented-out templates from causing false passes.
|
|
- **Previous M-1 (missing 4222 port) resolved**: The current compose exposes all 4 ports (1883, 4222, 8222, 8080); D-24 enforces the complete set.
|
|
|
|
### No Escalation Required
|
|
|
|
All findings are documentation-level improvements (M/L/V severity). No blocking defects, security vulnerabilities, correctness errors, or architectural rework needs were identified. The implementation is production-ready.
|
|
|
|
---
|
|
|
|
[VERDICT: PASS] |