Files
cc-ci/REVIEW-1b.md

6.4 KiB
Raw Blame History

REVIEW — Phase 1b (review & lint pass) — Adversary ledger

Phase plan (SSOT): /srv/cc-ci/cc-ci-plan/plan-phase1b-review-lint.md Loop state for THIS phase: STATUS-1b / BACKLOG-1b / JOURNAL-1b (Builder) · REVIEW-1b (Adversary, this file) · DECISIONS.md shared. Phase-1 STATUS.md/BACKLOG.md/REVIEW.md and the Phase-1c *-1c.md files are HISTORY — not this phase's state.

This phase the Adversary is also the white-box reviewer (§3 checklist), so this ledger holds both white-box review findings and the eventual cold RL3 re-verification of D1D10.

DoD I must independently confirm (RL1 lint-in-CI-green · RL2 §3 checklist run, blocking fixed · RL3 full cold D1D10 re-verify — the final gate · RL4 docs). Order per §2: tooling → review fixes → then RL3. Cardinal rule: never weaken a test to satisfy a lint/review nit; RL3 must confirm cleanup softened/skipped/regressed nothing.


Phase-1b orientation @2026-05-27 (Adversary cold start)

  • Pulled clean; Phase 1c is signed-off DONE (commit 6d2bc3d). Phase 1b kicked off by operator (manual transition).
  • Builder has not yet seeded STATUS-1b/BACKLOG-1b/JOURNAL-1b and has not claimed W0. No gate pending.
  • I began the independent white-box §3 review immediately (it's my role this phase and needs no Builder gate).

White-box §3 prep pass #1 @2026-05-27 — over post-1c codebase (PRE-cleanup baseline; advisory until RL3)

Recording the baseline state before any W0/W1 cleanup, so I can later confirm the cleanup regressed nothing.

  • Tests are real — PASS (provisional). Swept all 6 recipe suites (custom-html, lasuite-docs, keycloak, matrix-synapse, n8n, cryptpad) × install/upgrade/backup + conftest + runner/harness. No @pytest.mark.skip/xfail/skipif, no commented-out asserts, no tautologies. Install tests assert real app content (matrix: parses versions JSON non-empty; keycloak: admin DOM; others: Playwright body). Upgrade tests deploy v(n-1) → write marker → upgrade → assert exact marker survives. Backup tests establish+verify state → backup → mutate+verify → restore → assert exact pre-mutation state (keycloak deletes a realm). Watch-item (to re-check black-box at RL3): every upgrade test has a conditional pytest.skip() when no previous published version exists (e.g. custom-html test_upgrade.py:17-18). Valid by design, but if it ALWAYS skips, the upgrade stage would be silently fake — RL3 must confirm the upgrade stage actually RUNS (prev version found) for ≥1 recipe, not just skips. (1c E2E exercised this.)
  • Server state Nix-declared & idempotent — PASS (provisional). No .bootstrapped/run-once sentinels in modules/ or scripts/ (grep clean). Convergence/oneshot pattern per §9 to be re-read fully in pass #2.
  • No footguns / sleep — PASS (provisional). All time.sleep() in runner/harness/lifecycle.py (147,157, 212,238) + bridge.py (280) are poll-loop intervals inside while time.time() < deadline: loops, not bare readiness waits. wait_healthy polls converge-then-HTTP with timeouts. Teardown (lifecycle.py:215) is correctly ordered (undeploy → docker stack rm fallback → volumes/secrets while .env exists → drop .env last), retries volume removal, and verifies residual is empty (raises TeardownError otherwise).
  • No secrets in code/committed files — PASS (provisional). Grep for inline passwords/tokens/private-key blocks across .py/.nix/.sh/.yml clean (only env/file references + generators). Full leak re-verify (incl. published logs + dashboard, and generated app passwords) belongs to RL3 D6.

Still owed in white-box pass #2 (after I read the rest): harness DRY (recipe quirks in shared harness, not per-recipe copy-paste), log redaction real (bridge/dashboard/log pipeline), architecture matches plan (layout/§3, poll-primary trigger §4.1, traefik-is-coop-cloud-recipe §4.2; drift → DECISIONS.md).

W0 (RL1 — lint/format tooling + green) : PASS @2026-05-27 (Adversary cold)

Gate claimed in STATUS-1b. Acceptance: clean checkout → nix develop .#lint --command bash scripts/lint.shlint: PASS; lint stage wired in .drone.yml push pipeline. Verified cold, independently (no nix on sandbox; ran on cc-ci over a pristine tree, not the Builder's working copy):

  • Cold checkout = exact reviewed SHA. git archive 233939a (= my origin/main HEAD) piped to cc-ci → /tmp/ccci-cold (clean tree, no untracked/cached state, secrets submodule empty as lint excludes it). Not cloned from /root/cc-ci (that's a non-git plain copy) — archived from my own clone.
  • Lint PASS cold. HOME=/root nix develop .#lint --command bash scripts/lint.shexit 0, lint: PASS. All 8 linters ran clean: nixpkgs-fmt (0/14 reformat), statix, deadnix, ruff format (32 files), ruff check (all passed), shfmt, shellcheck, yamllint.
  • Stage real, not rigged. scripts/lint.sh genuinely invokes each linter in check mode and accumulates a fail flag → exit "$fail" (correct set -uo pipefail, no -e, so all run). The .drone.yml self-test push pipeline runs the exact command nix develop .#lint --command bash scripts/lint.sh and FAILs the build on non-zero. Toolchain pinned from nixpkgs in flake.nix (devShells.lint), so CI == local.
  • Gate has TEETH (break-it probe). Injected violations into the cold tree (a .py with import os,sys + x=1+2, and a mis-formatted .nix) → re-ran lint → exit 1, lint: FAIL (ruff E401/I001/F401 + nixpkgs-fmt). So the stage is not vacuously green.

Verdict: W0 PASS. Builder may proceed to W1. Advisory (not W0-blocking; re-confirm at RL3): Builder notes the Gitea→Drone push webhook is flaky (§4.1), so the lint stage may not auto-fire as a real Drone build on every push — RL1's intent ("future commits stay clean") depends on that path actually firing. The stage IS wired and proven green via its exact command; I'll confirm a real push triggers the Drone lint build when I re-verify M2/D-gates at RL3 (it overlaps). Not filing a finding now — bounded phase, acceptance-as-stated is met.

Status: W0 PASS logged. Awaiting Builder's W1 (review checklist) — I'll run my own white-box §3

pass #2 (harness DRY · log redaction · architecture-matches-plan) as independent prep / RL2 input, then the cold RL3 D1D10 re-verify LAST. Cardinal rule holds: cleanup must not weaken/skip/regress any test.