randomayzer/agents/codex/done/2026-10-19-auditor2-r5-review.md

52 lines
6 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Result: independent R5 review
Base SHA: `8284eb5d006943d42133eb665c99dcbc3133d6a6`
Reviewed SHA: `ca28be6ae629d3d67936475ea5896b6f70f6e1e4`
Result SHA: `ca28be6ae629d3d67936475ea5896b6f70f6e1e4` (review only; no source changes)
Files changed by this reviewer: this report and the task record only; no production or test files.
## Scope and evidence
Reviewed the immutable `8284eb5..ca28be6` diff, the updated stage-one owner manifest, and the Caddy/HAProxy templates, removed nginx template, R5 runbook, and local syntax validator. I did not inspect live VPS configuration, alter containers, or run the validator/full gates. The current checkout HEAD matched the reviewed SHA during review. `AGENTS.md` had a pre-existing local modification and was left untouched.
The runbook does a careful job of distinguishing observed configuration from examples. It requires operator discovery of exact proxy image/version, process arguments, mounts and include files, port 443 mode/SNI ordering, port 80 HTTP-01 path, and source/active configuration identity before editing. It backs up both active proxy files and host source; it also gives separate full-tree backups for directory mounts. Single-file inode preservation, post-edit active/source comparison, validation before reload, a two-protocol baseline for every operator-enumerated site, and scoped rollback are documented. The HAProxy snippets are additive, preserve existing route/default order, and use separate TLS passthrough and cleartext Caddy backends. The Caddy 2.8 site disables TLS-ALPN to require HTTP-01 and does not add access logging. The obsolete nginx alternative is removed.
## Findings
### MEDIUM — R5 runbook cannot execute the HAProxy apply step without a separate procedure
`docs/STAGE1_DEPLOYMENT_RUNBOOK.md:396-403` deliberately leaves the HAProxy reload command unspecified and directs the operator to find a deployment-specific mechanism, otherwise to stop. This is a sound safety guard against guessing at a shared proxy’s process model, but it means this deliverable does not itself provide the requested executable no-restart apply path even for the owner-confirmed Docker/HAProxy 3.0 deployment. The operator must research and supply a process-specific procedure before R5 can proceed; the current text correctly calls that an operational blocker. Parent reported an isolated official HAProxy 3.0 fixture where USR2 preserved the container identity/start time/restart count while replacing worker PIDs, but this does not prove the actual VPS runs master-worker mode or that its PID1/config mechanism accepts that signal. Do not transfer that fixture result to production.
### LOW — HAProxy route is reloaded before Caddy’s new site configuration
`docs/STAGE1_DEPLOYMENT_RUNBOOK.md:405-408` instructs applying the HAProxy route first and reloading Caddy second. During that interval, new TLS traffic for the hostname can be sent to a Caddy process that has not loaded the site block yet, producing a transient certificate or routing failure. A safer sequence is to reload Caddy first and then expose the new HAProxy SNI route, with validation/hash gates retained between steps. This is a narrow new-host availability window; it does not imply harm to existing sites.
No CRITICAL or HIGH findings in the reviewed R5 delta.
## Commands actually run
- `git rev-parse HEAD` — `ca28be6ae629d3d67936475ea5896b6f70f6e1e4`.
- `git status --short` — showed the pre-existing `AGENTS.md` modification and task-record files; no source edits from this review.
- `git show --stat --oneline --no-renames 8284eb5d006943d42133eb665c99dcbc3133d6a6..ca28be6ae629d3d67936475ea5896b6f70f6e1e4` — reviewed R5 delta summary.
- `git show ca28be6...:docs/STAGE1_DEPLOYMENT_RUNBOOK.md | nl -ba | sed -n '140,470p'` — inspected runbook and cited line references.
- `git show ca28be6...:deploy/randomayzer.haproxy.cfg | nl -ba` and `git show ca28be6...:deploy/randomayzer.Caddyfile | nl -ba` — inspected proxy snippets.
- `git show ca28be6...:agents/codex/notes/validate-r5-templates.sh | nl -ba` — inspected the local syntax-validator implementation; did not execute it.
- `git diff --check 8284eb5d006943d42133eb665c99dcbc3133d6a6 ca28be6ae629d3d67936475ea5896b6f70f6e1e4` — exit 0.
- `bash -n <(git show ca28be6ae629d3d67936475ea5896b6f70f6e1e4:agents/codex/notes/validate-r5-templates.sh)` — exit 0.
Coordinator reports that target-version Caddy/HAProxy validation and Docker R5 validator passed, and app gates plus PostgreSQL integration passed at identical runtime code. These are coordinator-reported results, not commands run by this reviewer. This review did not run npm tests, lint, build, the Docker validator, or any VPS operation.
## UNVERIFIED
- Actual VPS container IDs, image patch versions, entrypoints, config/include paths, mount sources, inodes and active/source equality.
- Actual HAProxy mode, TLS inspector, SNI precedence, catch-all ordering, cleartext port 80 route, HTTP-01 reachability, and graceful reload mechanism.
- Actual Caddy admin endpoint/reload availability, certificate issuance/renewal, DNS, WireGuard peer identity, firewall policy, login/callback behavior, and all-site baseline health.
- Directory-mount include trees are backed up, but the runbook does not give a concrete recursive active/source tree comparison command after edits; it requires the operator to compare the full set. This is non-blocking documentation debt if that comparison is completed before reload.
## Remaining blockers and non-blocking debt
- The MEDIUM runbook gap blocks live R5 application until a conditional, inspection-gated HAProxy reload procedure is documented or an operator supplies and records the verified deployment-specific method.
- The LOW reload-order issue should be corrected before live use.
- Actual topology and live acceptance remain the operator’s responsibility and unverified. No phase-closed or production-ready determination is made here.
Final verdict: the R5 configuration examples and safeguards are well-scoped, but the current runbook is not yet an executable end-to-end procedure for the HAProxy apply step. No live environment facts were inferred.