Files
cc-ci/machine-docs/REVIEW-1b.md

24 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.

White-box §3 pass #2 @2026-05-27 (Adversary, post-W0 formatted code) — RL2 input

Remaining §3 checklist items. No blocking findings.

  • Harness is DRY — PASS. Recipe quirks live in shared harness + per-recipe declarative metadata (tests/<recipe>/recipe_meta.py: HEALTH_PATH/HEALTH_OK/timeouts/EXTRA_ENV), consumed uniformly by tests/conftest.py (_recipe_meta, deployed/deployed_app fixtures) and runner/harness/lifecycle.py (_recipe_extra_env). No if recipe == "..." branches in the shared harness (the M6.5 no-surgery rule holds). Recipe-specific logic is isolated to that recipe's dir (e.g. keycloak kc_admin.py, cryptpad's derived SANDBOX_DOMAIN). Only smell: the ~6-8-line old_app upgrade fixture is copy-pasted across recipes — thin boilerplate over shared metadata; advisory, not a violation (factoring it would just add another per-recipe injection point). → IDEAS, not blocking.
  • Architecture matches plan — PASS. §4.1 trigger is poll-primary (bridge/bridge.py poll_loop runs unconditionally every ≤60s; webhook is optional + dedup'd by comment id; exact trimmed !testme; commenter-auth via read-level GET /orgs/{owner}/members/{user} 204=allow, fail-closed). §4.2 Traefik is the real coop-cloud/traefik recipe via abra (modules/proxy.nix: abra recipe fetch/app new traefik, WILDCARDS_ENABLED=1, compose.wildcard.yml, LETS_ENCRYPT_ENV="" → no ACME, cert as ssl_cert/ssl_key swarm secrets) — no hand-rolled traefik.nix. §3 layout matches.
  • Server state Nix-declared & idempotent — PASS. modules/proxy.nix deploy-proxy is Type=oneshot+RemainAfterExit, re-runs every activation and converges (insert secret only if absent, deploy). No .bootstrapped/run-once sentinels anywhere (grep clean, pass #1). Leans on 1c's already-proven D8 (byte-identical closure + live throwaway rebuild, no manual post-step).
  • Log redaction is real — PASS for infra secrets; one advisory gap to verify behaviorally at RL3/D6. runner/run_recipe_ci.py _redact_values() reads /run/secrets/* (≥8-char values) and run_stage_redacted() masks them in live-streamed stage output (sorted longest-first → no partial leak). But class-B generated app passwords are NOT under /run/secrets/*, so they are NOT in the _REDACT list — their non-leak rests entirely on the "harness never prints them / abra doesn't echo generated ones" assumption (code comment, run_recipe_ci.py:59-60). Also: the runner's own stdout (the cc-ci-run … Drone step) bypasses run_stage_redacted. This is exactly what my behavioral D6 leak test must catch at RL3 (grep published Drone logs and the dashboard for a known generated app password). Phase-1 D6 passed that test once; recording the white-box shape so RL3 re-checks it, not a new blocking finding. → WATCH-ITEM for RL3/D6.
  • Readability / docs accuracy — advisory; defer to RL4 (docs) + the ruff/lint pass already covers dead code / style deterministically.

Net of §3 white-box review (RL2 input): no blocking findings; 2 advisories (old_app copy-paste → IDEAS; app-secret redaction → RL3/D6 watch-item). I expect Builder's W1 to be light. I have NOT filed [adversary] BACKLOG items since nothing is blocking — will file if W1/RL3 surfaces a real defect.

Operator added RL5 + RL6 (plan §7, 2026-05-27) — both BLOCKING for 1b DONE. Noted; verification plan:

  • RL5 (Builder moves; Adversary verifies cold): modules/nix/modules/, hosts/nix/hosts/; flake.nix/flake.lock STAY at root so build ref #cc-ci is unchanged; fix flake internal paths + .drone.yml/scripts refs; update docs/architecture.md. Verification folds into RL3: a fresh recursive clone must still rebuild byte-identical to the running system (toplevel store hash WILL change — expected; what must hold is build==running + reproducible). I'll re-confirm cold at RL3.
  • RL6 (coordinated near-END-of-1b): move STATUS*/REVIEW*/JOURNAL*/BACKLOG*/DECISIONS.mdmachine-docs/; README.md stays at root (operator decision — human readme, not protocol). Update ALL refs (cc-ci-plan plans, AGENTS.md, .drone.yml, scripts). I verify refs updated + nothing broken. ⚠ CAVEAT affecting ME: the watchdog (launch.sh) reads STATUS-<id>.md/REVIEW-<id>.md at repo ROOT for handoffs/transitions — moving breaks it until launch.sh updated + watchdog restarted IN LOCKSTEP (orchestrator handles that). So I keep writing REVIEW-1b.md at root until the coordinated cutover, and at that moment I git mv my own REVIEW files (single-writer rule) in lockstep. Will NOT move them unilaterally or while a phase transition is pending.

RL2 (§3 white-box checklist) : PASS @2026-05-27 (Adversary)

My white-box passes #1+#2 found no blocking findings; Builder's own §3 self-review agrees. Advisories triaged (old_app copy-paste → IDEAS; generated-app-secret redaction → RL3/D6 watch-item). RL2 confirmed.

RL5 (nix/ consolidation) — structural PASS @2026-05-27; build-proof folds into RL3 below

  • modules/ and hosts/ gone from root; nix/modules/ (12 .nix) + nix/hosts/cc-ci/ (configuration.nix, hardware.nix) present; flake.nix + flake.lock stay at root (build ref #cc-ci unchanged). flake.nix imports ./nix/hosts/cc-ci/configuration.nix. No dangling ./modules/./hosts refs in flake.nix/.drone.yml/scripts (grep clean). docs/architecture.md + DECISIONS updated per Builder. The "flake still evaluates + builds byte-identical with new paths" proof = the cold rebuild in RL3 (below).

RL3 (final gate) — IN PROGRESS @2026-05-27 (Adversary cold). Re-verifying all D1D10; partial so far:

  • Cardinal rule — tests NOT weakened : PASS. Diffed every tests/**/test_*.py + runner/harness/ between pre-1b (6d2bc3d, the 1c-DONE commit) and HEAD. Every change is ruff line-wrapping only — assertion predicates, comparison operators (==, in), expected values, marker/SQL strings, and wait_healthy params are all byte-for-byte preserved (verified by reading the -w diff in full). No assertion removed/softened, no pytest.skip/xfail/assert True added, no test_ fn deleted. The format+RL5 cleanup regressed no test logic.
  • System health (cc-ci canonical) : confirmed. readlink /run/current-system == 8i3jcad9mrr01558lqckpi26nxn2ra3m-nixos-system-…50ab793 (matches claim); systemctl is-system-runningrunning; 5 infra stacks up (traefik[2 svc]/drone/ccci-bridge/ccci-dashboard/backups), no leftover test app (idle). [Note: "6 stacks" in 1c included a transient test app; 5 infra stacks is the idle baseline.]
  • D8 + RL5 byte-identical cold rebuild : PASS @2026-05-27 (Adversary cold, independent). On cc-ci: fresh git clone --recurse-submodules of origin to /tmp/ccci-rl3 (HEAD aa120d1, submodule secrets @2312f1c clean, secrets/secrets.yaml present) → nixos-rebuild build --flake "git+file:///tmp/ccci-rl3?submodules=1#cc-ci"toplevel 8i3jcad9mrr01558lqckpi26nxn2ra3m… == running (byte-identical, build==running). Proves D8 (reproducible from a fresh clone) and RL5 (new nix/ layout evaluates+builds, #cc-ci ref unchanged). Sanity: a build without ?submodules=1 fails secrets/secrets.yaml does not exist — confirms secrets genuinely come from the submodule, not baked in. Token used via transient -c http.extraHeader (not persisted in clone config — verified); temp clone removed.

Fresh live !testme e2e #1 — custom-html PR#2 (build #151, @2026-05-27) — D1/D2/D3/D7 PASS

Posted exact !testme (comment 13743, authorized org-member bot) @20:33:16Z. Bridge (poll 30s) → build #151 for PR-head db9a9502.

  • D1 PASS — triggered build for the PR head, latency 20s (<60s). Other comments don't trigger (only !testme matched; verified historically + exact-match code). Re-commenting re-ran (PR comment links to #151, an earlier identical comment linked to an older run #4 → re-run confirmed).
  • D2 PASS — install/upgrade/backup ran as separate reported stages, all green: install 2 passed (incl. playwright) 68.7s; upgrade test_upgrade_preserves_data PASSED 24.8s — it actually RAN, not skipped (resolves the pass#1 conditional-skip watch-item); backup test_backup_mutate_restore PASSED 42.9s. Real abra deploy/upgrade/backup-restore, no mocks.
  • D3 PASStest_playwright_page PASSED (real browser against the live app).
  • D7 PASS — bridge posted to PR#2: run for custom-html @ db9a9502 ✅ passed → drone.../cc-ci/151 (run link + outcome). Dashboard ci.commoninternet.net overview renders custom-html → success (YunoHost-CI-like badges; title "cc-ci — Co-op Cloud recipe CI").
  • D6 infra-secret leak : PASS — fetched #151 published step log; grepped each /run/secrets/* value (bridge gitea/drone tokens, drone_rpc_secret, webhook_hmac, drone_gitea_client_secret, test_secret, wildcard_cert, wildcard_key): 0 matches each; no echoed generated values / private keys; dashboard is a 21-line static status overview (structurally carries no secrets). (custom-html generates no app secrets, so the class-B app-password path is tested by e2e #2 below.)

D6 generated-app-secret WATCH-ITEM — RESOLVED (white-box) + behavioral check in flight

White-box: harness/abra.py secret_generate() runs abra app secret generate … -m via _run(), which subprocess.run(capture_output=True)the output (which holds the generated values) is captured and never printed (check=False, so no failure path re-emits it). So generated app secrets never reach the Drone log → that's why the proactive _REDACT (infra-only) gap is not a real leak. Residual advisory (theoretical): a check=True abra cmd that FAILS embeds its stdout/stderr in the raised AbraError msg, which pytest would print — only on failure, and abra status output isn't secret values; low risk, noting it. Behavioral confirmation in flight: e2e #2 = keycloak PR#1 (generates an admin password readable at /run/secrets/admin_password); watcher captures that exact value mid-run then greps the published log + dashboard for it (expect 0). Result logged on completion.

D4/D5/D8/D9/D10 — RL3 status

  • D4 (recipe-local tests) — discovery logic in run_recipe_ci.py is byte-identical (formatting- only) to the Phase-1 D4-passed version; custom-html ships no own tests/. Carried-forward; will note if the keycloak run exercises recipe-local discovery.
  • D5 (per-recipe tree + enroll)PASS. 6 trees present (custom-html/cryptpad/keycloak/lasuite- docs/matrix-synapse/n8n) + conftest.py; no test files deleted in 1b (git diff --diff-filter=D 6d2bc3d..HEAD -- tests/ empty); enroll documented in docs/enroll-recipe.md ("Copy from an existing recipe e.g. tests/custom-html/…", no-harness-surgery). Advisory: plan §3's literal tests/_template/ was never created (didn't exist pre-1b either — copy-existing-recipe used instead); pre-1b deviation, should be in DECISIONS — minor, not a 1b blocker.
  • D8 (reproducible server)PASS (byte-identical cold rebuild above).
  • D9 (docs)PASS. All 6 docs present (architecture/baseline/enroll-recipe/install/runbook/ secrets); README has the RL4 lint section (local + CI-enforced); architecture.md updated to the nix/ layout (RL4/RL5) and the 1c secrets model.
  • D10 (breadth, 6 recipes) — IN PROGRESS. Stance: test code + shared harness are byte-identical (formatting-only) and the closure is byte-identical to the one that produced the Phase-1/1c six- recipe green runs, so breadth carries forward; the cleanup-regression risk is covered by 2 fresh category-spanning green runs (custom-html=simple #151; keycloak=SSO/DB in flight). Will record the carry-forward set + this reasoning; can run additional recipes (sequentially) if the operator wants all 6 fresh.

Fresh live e2e #2 — keycloak PR#1 (build #152) — heavy SSO/DB recipe, D1/D2/D3 + D6-behavioral

  • D1 — build #152, latency 8s. D2 — full 3 stages green on a heavyweight SSO/DB recipe: install (test_realm_endpoint_healthy + test_playwright_admin_login, 446s), upgrade (test_upgrade_preserves_realm, 484s — ran), backup (test_backup_mutate_restore, 488s). D3 — playwright admin-login. Real keycloak + postgres, generated admin password + DB secrets.
  • D6 behavioral (app-secret) — PASS. keycloak generated an admin password (/run/secrets/admin_password)
    • DB creds during the run; published #152 log shows 0: BEGIN-PRIVATE-KEY, password assignments, echoed admin_password, secret-generate output, or standalone high-entropy tokens. Wildcard cert+key leak re-checked PROPERLY (my first grep mis-parsed the multi-line PEM as a flag — fixed; interior base64 line grep): 0 matches in BOTH #151 and #152. (Self-note: the buggy grep dumped the wildcard key into a sandbox /tmp task file — deleted immediately; never in repo/published/dashboard.)
  • D2 teardown guarantee — PASS. After both runs: no orphaned *-pr* stacks/volumes/secrets; system running, canonical still byte-identical 8i3jcad9.

RL3 — FULL COLD D1D10 RE-VERIFICATION : PASS @2026-05-27 (Adversary). Nothing weakened.

All re-verified on the cleaned + RL5 byte-identical closure (8i3jcad9==running==fresh-clone build), fresh evidence <24h. The lint/format + nix/ refactor regressed nothing.

D Verdict Evidence
D1 trigger PASS !testme→#151 (20s), #152 (8s); exact-match; re-comment re-ran
D2 matrix PASS custom-html + keycloak: install/upgrade/backup all green as separate stages; upgrade actually ran (not skipped); real abra deploy; teardown left no orphans
D3 py+playwright PASS playwright assertions green in both runs
D4 recipe-local PASS (carry-fwd) discovery code byte-identical (formatting-only) to Phase-1 D4-PASS impl
D5 test tree PASS 6 trees + conftest; enroll doc; no tests/ files deleted in 1b
D6 secrets PASS 8/8 infra-secret values + wildcard cert/key + generated keycloak admin pw: 0 in logs/dashboard; white-box: secret_generate output captured-never-printed
D7 results UX PASS PR comment w/ run link + passed; dashboard overview renders recipe statuses
D8 reproducible PASS fresh recursive clone → nixos-rebuild build …?submodules=1#cc-ci → toplevel 8i3jcad9==running
D9 docs PASS 6 docs present; README lint section (RL4); architecture.md = nix/ layout + 1c secrets model
D10 breadth PASS 2 fresh category-spanning green runs (custom-html=simple #151; keycloak=SSO/DB #152) + carry-forward of the Phase-1 Adversary-verified 6/6 set (cryptpad/lasuite-docs/matrix-synapse/n8n, builds #84#108) — test+harness+closure byte-identical, so breadth holds; cleanup-regression risk covered by the 2 fresh runs
Cardinal rule PASS 6d2bc3d..HEAD test diff is ruff line-wrapping only — no assertion/skip/test-fn change
RL5 PASS nix/ layout, flake at root (#cc-ci ref unchanged), byte-identical rebuild

Note on D10 scope: I did not re-run all 6 recipes fresh — that would be gold-plating against the bounded-phase discipline, since the 4 carried recipes use the byte-identical harness/test code against the byte-identical closure that produced their Phase-1 green runs, so a re-run carries ~zero regression signal beyond the 2 fresh runs already done. If the operator wants strict 6/6-fresh, I can run the remaining 4 sequentially on request.

RL6 — protocol files → machine-docs/ : PASS @2026-05-27 (Adversary, lockstep cutover)

The coordinated cutover executed cleanly:

  • Orchestrator lockstep done. cc-ci-plan/launch.sh now has resolve_state() (lines 67-69) that prefers machine-docs/<file> and falls back to root — so the watchdog survives the move and stays move-agnostic. Proof it works post-move: the watchdog pinged me for the RL6 gate from machine-docs/STATUS-1b.md (it read the moved file). Handoff intact.
  • Builder moved (commit 992d87c): STATUS*.md/BACKLOG*.md/JOURNAL*.md (3 each) + DECISIONS.mdmachine-docs/. README.md correctly LEFT at repo root (operator decision).
  • Adversary moved (this commit, single-writer rule): REVIEW-1b.md + REVIEW.md + REVIEW-1c.mdmachine-docs/. Root now holds only README.md (+ flake/nix/code); no protocol file left at root.
  • References re-verified. README "Loop state" section updated → "lives under machine-docs/"; docs/install.mdmachine-docs/DECISIONS.md. No .drone.yml / scripts/ / flake.nix / nix/hosts references to protocol files (grep clean) ⇒ the build closure is unaffected (cc-ci still running, byte-identical 8i3jcad9 — RL6 is a repo-doc move, touches no nix input).
  • Trivial advisory (non-blocking): 4 See DECISIONS.md bare-name comment refs in nix/modules/{drone,drone-runner,proxy}.nix aren't path-qualified to machine-docs/ — but they were never path-qualified pre-move (always bare "DECISIONS.md"), the file is still findable by name, and README states its location. Optional tidy (prefix machine-docs/), not an RL6 failure. → IDEAS.

Verdict: RL6 PASS.

🏁 ADVERSARY FINAL SIGN-OFF — Phase 1b : ALL RL1RL6 Adversary-PASS @2026-05-27. NO VETO.

RL Verdict
RL1 lint/format in CI + green PASS (cold, with break-it teeth)
RL2 §3 white-box checklist PASS (no blocking findings)
RL3 full cold D1D10 re-verify PASS (nothing weakened; byte-identical closure; 2 fresh e2e; leak-clean)
RL4 docs PASS
RL5 nix/ consolidation PASS (byte-identical rebuild)
RL6 machine-docs/ move PASS (watchdog-survived lockstep)

No open [adversary] findings; advisories triaged to IDEAS (old_app copy-paste; _template deviation; bare-name DECISIONS refs) + one documented RL1 advisory (flaky Gitea→Drone push webhook — lint stage is wired + proven via its exact command, auto-fire needs the operator's webhook; non-blocking). The Builder is cleared to write ## DONE to machine-docs/STATUS-1b.md. Once DONE is written, the DONE handshake holds (every RL has a <24h Adversary PASS, no VETO) and the 1b loop terminates.