praxis/.ciagent/REVIEW.md

Praxis v0.2 Milestone Review — Proxmox LXC Deployment

Reviewer: ci-code-reviewer (multi-persona) Branch reviewed: milestone/v0.2-lxc-deploy (vs main) Date: 2026-08-03 Files changed: 44 (6,349 insertions, 932 deletions) Test suite: 121 bats tests — 121 passing (after P0 fixes)

---

1. Review Summary

Verdict: APPROVE_WITH_NOTES

The v0.2 milestone delivers a clean, well-documented Proxmox LXC deployment pipeline adapted from the proven coreci pattern. The code is consistently POSIX-sh, idempotent, and backed by a thorough bats suite (121 tests) that exercises the real orchestrator logic with mocked siblings + a live e2e suite gated behind PRAXIS_E2E_LIVE=1. The G-101 token-baking fix is correct and the secret-injection chain is consistent across all three layers (lxc-config → install-service → docker-compose env_file).

Two P0 (blocking) issues were found and fixed in the working tree: both were test/code drift where the bats expectations no longer matched the production defaults in lxc-config.sh / .env.example. After the fixes, all 121 tests pass. Eight P1+ issues are flagged for post-hoc review — none block ship.

Severity	Count	Action
P0 (critical)	2	Fixed in working tree (do not commit per instructions)
P1 (important)	3	Flagged for post-hoc review
P2 (nit)	5	Flagged for post-hoc review

---

2. Per-Axis Findings

2.1 Correctness

Correct:

• The deploy orchestrator (lxc-deploy.sh) correctly sequences stage → clone → config → start → health-check, with a trap-based rollback that captures $? so set -e child failures trigger rollback (not just INT/TERM). The trap is installed AFTER vmid is resolved and BEFORE clone — so a stage-snippet failure (pre-trap) correctly does not invoke rollback (nothing to roll back). This ordering is documented in the test stage-snippet fails (set -e) → ... (trap not yet installed).
• Idempotency (D-027) is correctly implemented: healthy+running → skip; exists+unhealthy → error with --recreate/--reconfigure guidance (CT left intact); --reconfigure re-PUTs config + restarts (no clone); --recreate rolls back + redeploys.
• pve_poll correctly accepts WARNINGS N (non-fatal warnings, e.g. systemd 255 nesting hint) in addition to OK — this is a real Proxmox behavior that a naive == "OK" check would break on.
• health-check.sh correctly uses (.inet? // .ip? // empty) and grep -v '^$' to skip hwaddr (the P18 coreci bug where head -1 picked the MAC). The comment documents the fix.
• lxc-config.sh sed-cleanup pattern is idempotent: removes prior hookscript:/onboot:/lxc.environment: PRAXIS|GITEA_TOKEN|DEEPGRAM| CARTESIA|OLLAMA lines before appending fresh ones. Verified by the idempotent — re-run does not duplicate test.
• db/migrate.py + db/store.py both read PRAXIS_DB_PATH from env (G-102 fix) — consistent with docker-compose.yml's PRAXIS_DB_PATH: /app/data/praxis.db + the volume mount.

Issues:

• P0-1 (FIXED): test/lxc-config.bats:175-181 expected stale defaults (OLLAMA_BASE_URL=http://ollama.cloudinit.dev:11434, DEEPGRAM_LANGUAGE=en-US, DEEPGRAM_REGION=us-east-1) that do NOT match the production code (lxc-config.sh:66,73,74), .env.example, docker-compose.yml, ARCHITECTURE.md, or PLAN.md — all of which use https://ollama.com/v1, en, na. The test was failing. Fixed: aligned the test expectations with the production defaults.
• P0-2 (FIXED): test/lxc-deploy.bats:222-230 ("PROXMOX_LXC_VMID set → use the configured VMID") was failing because lxc-deploy.sh:51-64 sources ~/coreci/.ciagent/.env.secrets + ${PROJ_ROOT}/.ciagent/ .env.secrets when present, and on a live deploy host those files set PROXMOX_LXC_VMID=auto — overriding the test's PROXMOX_LXC_VMID=300. The test sandbox did not isolate HOME or PROJ_ROOT. Fixed: the test now exports HOME="${STUB_DIR}" so neither secrets file is found, and the deploy script falls back to the exported test env (emitting its "WARNING — not found" message, which is harmless).

2.2 Testing

Correct:

• 121 bats tests across 8 suites (api, lxc-clone, lxc-config, lxc-start, lxc-deploy, health-check, rollback, stage-snippet, firstboot-hook) + 1 live e2e suite (gated by PRAXIS_E2E_LIVE=1).
• Tests exercise the REAL scripts with mocked siblings + a real ct-exists.sh (P16) — the orchestrator logic (trap, sequencing, idempotency, flag parsing) is genuinely verified, not stubbed.
• Edge cases covered: empty/null UPID, 503 retry exhaustion, WARNINGS exitstatus, hwaddr-vs-IP, idempotent re-run, missing-arg usage errors, env-validation failures, branch-fallback in clone, snippet-already- staged short-circuit.
• The setup_helper.bash shared sandbox is clean and reusable.
• The live e2e suite has a skip guard with a clear message + a teardown that rolls back any leftover CT — safe to run bats scripts/proxmox/test/ in CI without a live cluster.

Issues:

• P1-1: test/lxc-deploy.bats sandbox isolation (the P0-2 fix) is fragile: it relies on HOME redirect, but PROJ_ROOT is computed by cd "${SCRIPT_DIR}/../.." where SCRIPT_DIR is the sandbox <ROOT>. If <ROOT>'s parent layout ever changes, PROJ_ROOT could resolve to a real repo root. A more robust fix would be to patch the deploy script's CORECI_SECRETS/PRAXIS_SECRETS paths via an env override (e.g. PRAXIS_SECRETS_PATH), or to copy a no-op .env.secrets into the sandbox. Flag for post-hoc review.
• P1-2: No bats test for timing.sh (the comment in lxc-deploy.bats says "timing.sh itself is tested in timing.bats" but no such file exists in the diff). timing.sh has non-trivial logic (the _TIMING_STARTS string-map scan + the node_exporter textfile collector). Flag for post-hoc review — add a timing.bats.
• P1-3: No test for install-service.sh (runs inside the CT). It writes the env file + systemd unit + starts the service. The firstboot-hook.bats verifies it's invoked but not its behavior (env-file shape, systemd unit content, idempotency). Flag for post-hoc review — a sandboxed test with mocked systemctl/useradd would close this gap.

2.3 Security

Correct:

• G-101 token baking is sound. stage-snippet.sh:64 sed-substitutes the literal ${GITEA_TOKEN} placeholder in the fetched snippet with the real token. The baked snippet lives only in Proxmox snippet storage (local:snippets/praxis-firstboot.sh), NOT in git. The hookscript runs on the PVE host where lxc.environment is invisible, so baking is the correct mechanism. The | sed delimiter avoids = (base64 padding) and / (common in URLs).
• Secrets are not committed. .ciagent/.env.secrets is mode 0600 and in .gitignore (with !.env.example exception for the template). .dockerignore excludes .env, .env.secrets, .env.* (with !.env.example) so secrets never enter the image.
• install-service.sh:65-66 writes /etc/praxis/server.env as root:praxis 0640 — group-readable by the service user, not world.
• The lxc-config.sh SSH step uses StrictHostKeyChecking=no — acceptable for an automated deploy pipeline on a trusted cluster, but see P2-1.
• docker-compose.yml uses env_file: required: false for /etc/praxis/server.env so docker compose config validates in dev without the file, but install-service.sh always creates it before docker compose up in production.

Issues:

• P2-1: lxc-config.sh:92 uses ssh -o StrictHostKeyChecking=no. This is the standard pattern for automated deploys to a known PVE host, but it accepts any host key on first connect. For defense-in-depth, consider ~/.ssh/known_hosts pre-seeding or StrictHostKeyChecking=accept-new (accepts + pins on first connect, fails on subsequent changes). Nit — the threat model (single-node PVE, operator-controlled) likely accepts this.
• P2-2: stage-snippet.sh:46 puts GITEA_TOKEN in the Gitea raw URL query string (?token=${GITEA_TOKEN}). The comment acknowledges this is "acceptable for an automated deploy pipeline." The token could appear in web server access logs on the Gitea host. Gitea's ?token= is the documented way to access private repos via raw URL, so this is a known tradeoff. Nit — consider Authorization: token <TOKEN> header instead if Gitea supports it for raw file access (would require a two-step fetch: header-based GET to a local file, then upload).

2.4 Performance

Correct:

• Dockerfile layer caching is correct. Stage 1: COPY package.json package-lock.json → npm ci → COPY client/ → npm run build. Stage 2: COPY pyproject.toml README.md → pip install . → COPY server/ scenarios/ db/ → COPY --from=client-builder. Deps are cached; source changes don't invalidate the pip/npm layers. This is the G-105 fix and it's done right.
• Multi-stage build keeps the final image small (no node, no build tools, no client source — only the built dist).
• pve_get 503 retry is bounded (3 attempts, 2s backoff) — used only for idempotent reads, NOT mutating calls.
• pve_poll is bounded (120 × 2s = 4 min max) — prevents infinite hangs.
• health-check.sh polls with --connect-timeout 2 per attempt + a 600s total budget (G-104 fix for Docker build margin).

Issues:

• P2-3: Dockerfile:39 runs pip install --no-cache-dir . with only pyproject.toml + README.md copied. pip install . on a pyproject-only context (no source) works because setuptools reads pyproject.toml for metadata + deps, but it will FAIL if any dep tries to import the package during install (none do here — fastapi/uvicorn/ pipecat don't import praxis). This is correct for now but fragile if a future dep adds a praxis import in its setup. Nit — consider pip install --no-cache-dir -e . after copying source, or split deps into a requirements layer. Documented as the G-105 tradeoff.
• P2-4: stage-snippet.sh:88-93 spawns a python3 -m http.server + a ( sleep 60 && kill ) safety net. The server is killed after the upload completes (line 117), but the sleep 60 subprocess is NOT killed — it lingers for up to 60s after the script exits. Harmless (it just tries to kill an already-dead PID), but slightly sloppy. Nit — capture the sleep's PID and kill it on EXIT.

2.5 Maintainability

Correct:

• Every script has a clear header comment block: purpose, env vars (required + optional with defaults), args, exit codes. The lxc-config.sh header documents the G-101 reasoning (why SSH vs REST for hookscript/lxc.environment) — excellent for future readers.
• Consistent with coreci patterns (sourced api.sh, pve_env validation, UPID polling, trap-based rollback) while cleanly diverging where praxis differs (no proxy tier, Docker-in-LXC vs Go binary, praxis env var names). The divergences are documented in test comments ("Praxis v0.2 vs coreci key differences asserted here").
• timing.sh is a clean adaptation of the coreci timing helper with praxis-prefixed metrics. The POSIX-sh string-map (no associative arrays) is well-commented.
• e2e-deploy.sh is a good integration capstone — loads secrets, runs the deploy, verifies /health + client HTML serving.

Issues:

• P2-5: lxc-config.sh:124-130 builds a remote shell snippet via ssh ... "conf='${conf_file}'; sed -i '...'; cat >> ...". The sed -i expression uses ;-separated delete patterns (/^hookscript:/d;/^onboot:/d;/^lxc\.environment: PRAXIS/d;...). This is correct but hard to read. A future maintainer adding a new env var group (e.g. WHISPER_) must update BOTH the append_lines function AND the sed delete pattern, or risk stale lines surviving re-config. Consider a single sed -i '/^lxc\.environment:/d' (drop ALL lxc.environment lines) since append_lines always re-emits the full set. Nit — document the dual-update requirement in a comment.

---

3. P0 Issues (Critical — Fixed in Working Tree)

P0-1: lxc-config.bats expected stale OLLAMA/DEEPGRAM defaults (FAILING TEST)

• File: scripts/proxmox/test/lxc-config.bats:175-181
• Symptom: Test 76 failed: grep '^lxc.environment: OLLAMA_BASE_URL=http://ollama.cloudinit.dev:11434$' did not match.
• Root cause: The test expected http://ollama.cloudinit.dev:11434, en-US, us-east-1 — stale values from an earlier draft. The production code (lxc-config.sh:66,73,74), .env.example, docker-compose.yml, ARCHITECTURE.md, and PLAN.md all consistently use https://ollama.com/v1, en, na. The test drifted.
• Fix applied: Aligned the test grep patterns with the production defaults (https://ollama.com/v1, en, na).

P0-2: lxc-deploy.bats "PROXMOX_LXC_VMID set" test failed due to secrets-file leakage (FAILING TEST)

• File: scripts/proxmox/test/lxc-deploy.bats:222-230
• Symptom: Test 88 failed: grep 'deploy: using configured VMID 300' did not match.
• Root cause: lxc-deploy.sh:51-64 sources ~/coreci/.ciagent/.env.secrets and ${PROJ_ROOT}/.ciagent/.env.secrets when present. On a live deploy host (this review ran on the actual cluster), the coreci secrets file sets PROXMOX_LXC_VMID=auto, overriding the test's PROXMOX_LXC_VMID=300. The test sandbox did not isolate HOME or PROJ_ROOT, so the real secrets file leaked into the test.
• Fix applied: The test now exports HOME="${STUB_DIR}" so neither secrets file is found; the deploy script falls back to the exported test env (emitting its "WARNING — not found" message, which is harmless and does not affect the test assertions). All other lxc-deploy.bats tests continue to pass with this change.

After both fixes: 121/121 bats tests pass.

---

4. P1+ Issues (Flagged for Post-Hoc Review)

P1-1: lxc-deploy.bats sandbox isolation is fragile

• File: scripts/proxmox/test/lxc-deploy.bats (the P0-2 fix)
• Issue: The HOME redirect works but relies on PROJ_ROOT (computed via cd "${SCRIPT_DIR}/../..") resolving to a path with no .ciagent/.env.secrets. If the sandbox layout changes, this could break. A more robust fix: add an env override to lxc-deploy.sh (e.g. PRAXIS_SECRETS_PATH / CORECI_SECRETS_PATH) so tests can point at a no-op file, or copy a no-op .env.secrets into the sandbox.

P1-2: No bats test for timing.sh

• File: (missing) scripts/proxmox/test/timing.bats
• Issue: lxc-deploy.bats:104-107 stubs timing.sh to a no-op and comments "timing.sh itself is tested in timing.bats" — but no timing.bats exists in the diff. timing.sh has non-trivial logic (the _TIMING_STARTS string-map scan, duration computation, optional node_exporter textfile collector). Add a timing.bats covering: start/end pairing, duration math, stray timing_end with no start (no-op), textfile collector write when NODE_TEXTFILE_COLLECTOR_DIR is set + writable.

P1-3: No test for install-service.sh

• File: scripts/install-service.sh
• Issue: firstboot-hook.bats verifies install-service.sh is invoked via pct exec, but does not test its behavior: env-file shape (/etc/praxis/server.env content), systemd unit content, user creation, idempotency. A sandboxed test with mocked systemctl/ useradd/apt-get would close this gap and catch drift in the env-file format (which must match docker-compose.yml's env_file expectations).

P2-1: ssh StrictHostKeyChecking=no

• File: scripts/proxmox/lxc-config.sh:92
• Issue: Accepts any host key on first connect. Consider StrictHostKeyChecking=accept-new (pins on first connect, fails on subsequent changes) for defense-in-depth. Acceptable for the current single-node-PVE threat model.

P2-2: GITEA_TOKEN in Gitea raw URL query string

• File: scripts/proxmox/stage-snippet.sh:46
• Issue: ?token=${GITEA_TOKEN} could appear in Gitea access logs. Documented as an accepted tradeoff. Consider header-based auth if Gitea supports it for raw file access.

P2-3: Dockerfile pip install . without source

• File: Dockerfile:38-39
• Issue: pip install --no-cache-dir . with only pyproject.toml + README.md works because no dep imports praxis at install time. Fragile if a future dep does. Documented as the G-105 tradeoff.

P2-4: stage-snippet.sh sleep 60 subprocess lingers

• File: scripts/proxmox/stage-snippet.sh:91
• Issue: The ( sleep 60 && kill ) safety-net subprocess is not killed when the HTTP server exits. It lingers up to 60s trying to kill an already-dead PID. Harmless but sloppy. Capture + kill the sleep PID on EXIT.

P2-5: lxc-config.sh sed delete pattern must be kept in sync with append_lines

• File: scripts/proxmox/lxc-config.sh:127
• Issue: The sed -i '/^hookscript:/d;/^onboot:/d;/^lxc\.environment: PRAXIS/d;...' pattern must be updated whenever a new env-var GROUP is added to append_lines, or stale lines survive re-config. Consider a single sed -i '/^lxc\.environment:/d' (drop ALL lxc.environment lines) since append_lines always re-emits the full set. Document the dual-update requirement.

---

5. Positive Observations

1. Test suite quality is high. 121 bats tests exercising real orchestrator logic (not stubbed) with a shared sandbox helper, edge cases (503 retry, WARNINGS exitstatus, hwaddr-vs-IP, idempotent re-run, empty/null UPID), and a properly-gated live e2e suite. This is the strongest part of the milestone.

2. G-101 token baking is correct and well-documented. The stage-snippet.sh sed substitution + the firstboot-hook.sh ${GITEA_TOKEN} placeholder + the lxc-config.sh header explaining why SSH is needed (lxc.environment invisible to host-side hookscript) form a coherent, secure secret-injection chain.

3. Idempotency is thorough. lxc-deploy.sh (CT exists + healthy → skip; unhealthy → guidance + --recreate/--reconfigure), lxc-config.sh (sed-cleanup before append), rollback.sh (404-tolerant), firstboot-hook.sh (skip if /opt/praxis/.git + service active), stage-snippet.sh (snippet-already-staged short-circuit). Every layer is re-runnable.

4. Dockerfile layer caching is correct (G-105). Deps installed before source copy; multi-stage build keeps the image small. The client/package.json → npm ci → client/ pattern in Stage 1 mirrors the server pattern.

5. Consistent with coreci, cleanly divergent where needed. The api.sh / pve_env / pve_poll / trap-rollback patterns are inherited from the proven coreci pipeline; the divergences (no proxy tier, Docker-in-LXC vs Go binary, praxis env var names, 600s health timeout) are documented in script headers + test comments.

6. Shellcheck-clean. All scripts pass shellcheck with only expected SC1090 (non-constant source) warnings on the dynamic . "$SECRETS" sourcing.

7. Documentation is excellent. Every script has a purpose + env + args + exit-code header. The lxc-config.sh header explains the REST-vs-SSH split for root-only fields. The rollback.sh header notes the proxy-tier removal for future readers.

---

6. Summary

Axis	Verdict
Correctness	✅ (2 P0 test-drift bugs fixed)
Testing	✅ (121 passing; 3 gaps flagged P1)
Security	✅ (G-101 sound; no secrets committed)
Performance	✅ (Dockerfile caching correct; bounded retries/polls)
Maintainability	✅ (well-commented; 1 sync-burden flagged P2)

Overall: APPROVE_WITH_NOTES — ship after committing the 2 P0 test fixes. The 8 P1+ items are non-blocking improvements for future slices.

---

Generated by ci-code-reviewer (multi-persona) on 2026-08-03.