recipe-maintainers/cc-ci
0
0
Fork 0
Code Issues 1 Pull Requests 3 Actions Packages Projects Releases Wiki Activity
Files
85d14101efc1c85aca950dd8fc9be5094da00d43
cc-ci/REVIEW-rcust.md

20 KiB
Raw Blame History

REVIEW-rcust.md — Adversary ledger for the recipe-customization restructure phase

SSOT for this phase: /srv/cc-ci/cc-ci-plan/recipe-custom-restructure-full-plan.md. Gates: M1 (implementation verified — branch restructure/recipe-custom, unit+concurrency+lint green on cold clone, resolved-customization diff clean for all 21 recipes, adversarial diff review) and M2 (merged + real-CI regression sweep matching baseline matrix). DONE requires fresh PASS for both with no open VETO.

I own this file and the ## Adversary findings section of BACKLOG-rcust.md only.

Standing watch items (what I will hunt at M1/M2)

  • Coverage loss (cardinal risk): for every migrated recipe, old loaders' effective customization values must equal new meta.load() values. Throwaway diff script over all 21 recipe dirs; any delta = finding.
  • Assertion weakening in tests/<recipe>/ diffs — migrations must be mechanical only (signatures, fixture/key renames, underscore prefixes). Any changed assert/expected value = VETO.
  • Deleted-code fallout — dangling refs to _recipe_meta, _load_meta, _recipe_extra_env, _recipe_meta_flag, declared_deps, is_canonical_enrolled, OIDC_AT_INSTALL, CHAOS_BASE_DEPLOY, SKIP_GENERIC, setup_custom_tests, deps_apps, deps_creds, deployed_app.
  • Validation gaps — typo'd key / wrong type / callable-on-data-key must raise MetaError, not pass.
  • R2 fixed end-to-end — orchestrator load path delivers SCREENSHOT to screenshot.py.
  • HC2 / F2-11 integrity — repo-local default-deny, requires_deps skip-report, generic floor semantics all unchanged.

Verdicts

(no GATE verdict yet — M1 is not claimed. M1 only claims after P1–P6 are all on the branch; Builder has landed P1 (472a68b) + P2 (8cd72fd) and is mid-P3. The interim pre-review below is front-loaded break-it work on the FROZEN P1/P2 commits — NOT an M1 PASS.)

Interim pre-review of frozen P1+P2 (branch @ 8cd72fd) — @2026-06-10, cold from upstream clone

Done as idle-time break-it work while no gate is pending. P1/P2 phase commits won't be rewritten (Builder adds P3+ on top), so reviewing them now is non-wasted and front-loads M1. Cold clone of origin/restructure/recipe-custom into /tmp/rcust-verify from the true upstream remote.

No defects found so far. Results:

  1. Deleted-code fallout — CLEAN. Grepped runner/ tests/ scripts/ for live refs to every deleted symbol (_recipe_meta, _load_meta, _recipe_extra_env, _recipe_meta_flag, declared_deps, is_canonical_enrolled, OIDC_AT_INSTALL, CHAOS_BASE_DEPLOY, SKIP_GENERIC, setup_custom_tests, deps_apps, deps_creds, deployed_app). All hits are comments/docstrings explaining the deletion, test names, or the intentionally-RETAINED CCCI_SKIP_GENERIC* env form (kept per P2c). Zero live call-sites. setup_custom_tests.sh files gone.
  2. All-recipes-load-clean (typo gate) — PASS, independently. Ran meta.load() (pure stdlib) over all 21 recipe dirs cold via plain python3 (did NOT trust the Builder's test_meta.py). All 21 load; non-default key sets sane. Every ALL-CAPS key used in any recipe_meta.py is in the 14-key registry.
  3. Coverage-loss diff (CARDINAL check) — ZERO deltas on data keys + hook presence. Throwaway harness (/tmp/diff_meta.py) reproduces main's six-loader effective resolution (_load_meta, declared_deps, is_enrolled, _recipe_extra_env) from MAIN's recipe_meta files and diffs vs the BRANCH's meta.load() for all 21 recipes. After correcting one harness artifact (EXTRA_ENV default is {} not None), 0/21 recipes show any delta for HEALTH_PATH/HEALTH_OK/DEPLOY_TIMEOUT/ HTTP_TIMEOUT/BACKUP_CAPABLE/EXPECTED_NA/UPGRADE_BASE_VERSION/DEPS/WARM_CANONICAL + presence of READY_PROBE/BACKUP_VERIFY/UPGRADE_EXTRA_ENV/EXTRA_ENV/SCREENSHOT.
  4. Validation gaps — CLOSED. Crafted tmp recipe_metas: typo'd key → MetaError (with "did you mean DEPLOY_TIMEOUT?"); wrong type (DEPLOY_TIMEOUT="str") → MetaError; callable on data key (DEPLOY_TIMEOUT=lambda ctx:...) → MetaError; _PRIVATE/lowercase-helper → loads clean (exemption works). All four behave per the locked decision.
  5. meta.py read — single exec(), frozen RecipeMeta generated from KEYS, _coerce rejects bool-as-int and callable-on-data-key; non_default compares vs registry default. No issues.

Still UNVERIFIED for M1 (do NOT treat above as M1 PASS): full pytest tests/unit -q + pytest tests/concurrency -q + scripts/lint.sh cold on the cc-ci host; R2 end-to-end through the real orchestrator screenshot path; P3 ctx-hook signature migration (assert byte-identical, legacy lambda domain: raises clear MetaError); P4/P5/P6; re-run the coverage diff on the FINAL branch (P3 changes hook signatures); recipe-test diffs are mechanical-only (no assertion weakening); HC2/F2-11/generic-floor integrity. These wait for the claim(rcust): M1.

Interim pre-review of frozen P3 (branch @ fd02d9f) — @2026-06-10, cold from upstream clone

Builder landed P3 (uniform ctx hook convention) and moved to P4, so P3 is frozen. Pre-reviewed it. No defects found.

  1. Mechanical-migration discipline — HELD (no VETO trigger). git diff 8cd72fd..fd02d9f over tests/*/ shows ZERO changed assert/expected literals. Every hook change is purely def HOOK(domain[, meta]) → def HOOK(ctx) + domain → ctx.domain in the body. Spot-checked cryptpad/mumble/ghost/lasuite-drive recipe_meta.py + lasuite-drive ops.py: seeded values, return dicts, paths, status codes, and the pre_restore assert _psql(...) in (...) are byte-identical apart from the ctx. deref.
  2. HookCtx — present + complete. meta.HookCtx frozen dataclass has all 5 documented fields (.domain, .base_url, .meta, .deps, .op); meta.hook_ctx(domain, meta, op=…) factory builds it and pulls deps from $CCCI_DEPS_FILE. All call sites migrated: run_recipe_ci pre_<op>, BACKUP_VERIFY; lifecycle extra_env + READY_PROBE; screenshot SCREENSHOT(page, ctx). (NB my first pass falsely flagged "no HookCtx" — that was a STALE WORKTREE at P2; corrected by checking out fd02d9f. Logged here for honesty.)
  3. Legacy-signature guard (P3.4) — PRESENT + works, live-probed. meta.check_hook_signature exact-matches positional params and raises a CLEAR MetaError naming the P3 migration + HookCtx fields. Wired into both load() (recipe_meta hooks; SCREENSHOT expects (page, ctx), rest (ctx)) and the orchestrator (ops.py pre_<op>). Crafted tmp metas: legacy READY_PROBE(domain), SCREENSHOT(page, domain, meta), EXTRA_ENV(domain) all → MetaError at load; READY_PROBE(ctx) loads clean. No silent mid-run TypeError path.
  4. Coverage diff re-run at P3 head — still 0/21 deltas (hook presence + all data keys unchanged).

Net: P1+P2+P3 all clean under cold adversarial probing. M1 still gated on full unit+concurrency+lint on the cc-ci host, P4–P6, R2 end-to-end via the real screenshot orchestrator path, and a final coverage re-diff. No findings filed; no VETO.

Interim pre-review of frozen P4 (branch @ 29a28e2) — @2026-06-10T18:55Z, cold from fresh host clone

Builder landed P4 (custom-test ergonomics) and moved to P5, so P4 is frozen. Pre-reviewed it cold. No defects found. NOT an M1 verdict — M1 stays gated (see "Still UNVERIFIED" below).

Cold acceptance (fresh git clone on cc-ci host at 29a28e2, my own checkout — not the Builder's):

  • cc-ci-run -m pytest tests/unit -q → 184 passed (exact match to claim; full suite, no cross-fixture pollution from the session-scoped deps fixture).
  • cc-ci-run -m pytest tests/unit/test_discovery.py test_discovery_phase2.py test_conftest_fixtures.py -q → 14 passed.
  • nix develop .#lint --command scripts/lint.sh → lint: PASS (ruff format/check, deadnix, shfmt, shellcheck, yamllint all clean).

Correctness probes:

  1. Placement-rule claim ("zero in-repo users of top-level custom tests") — HOLDS. Filesystem sweep of every tests/<recipe>/test_*.py: ALL are lifecycle names (test_{install,upgrade, backup,restore}.py). No top-level non-lifecycle custom exists in-repo, so dropping the top-level glob in discovery.custom_tests loses ZERO coverage. The lifecycle-name exclusion is retained inside functional/playwright as the double-run safety net.
  2. Discovery diff — clean. Top-level glob(test_*.py) branch removed; functional/ + playwright/ subdir globs retained with basename not in lifecycle_names guard. Docstring + module header updated to state the placement RULE.
  3. Test changes are adaptation + strengthening, NOT weakening (no VETO trigger).
    • test_discovery_phase2: renamed to ..._placement_rule_...; now ASSERTS the top-level test_sso_smoke.py is not in names (new negative assertion proving the behavior change), while functional/playwright customs are still in names and lifecycle name excluded.
    • test_discovery::test_custom_tests_repo_local_gated: repo-local custom moved from top-level into functional/; HC2 default-deny (== [] when unapproved) and approved-case (functional/test_sso.py in names, test_install.py excluded) both INTACT. HC2 integrity preserved.
  4. op_state fixture — correct. Skips with clear reason on unset env / missing file / non-JSON (except ValueError catches JSONDecodeError); reads & returns parsed dict otherwise. Tests cover 3 of 4 paths (the non-JSON skip path is untested — minor coverage gap, not a defect; the branch is trivially correct by inspection).

Net: P1+P2+P3+P4 all clean under cold adversarial probing; both halves of every phase claim (unit count + lint) reproduced cold on a fresh clone. No findings filed; no VETO.

Still UNVERIFIED for M1 (do NOT treat above as M1 PASS): P5 (manifest) + P6 (docs); pytest tests/concurrency -q cold; R2 end-to-end through the real orchestrator screenshot path; final coverage re-diff on the COMPLETE branch (P1–P6, all 21 recipes, effective customization set unchanged); recipe-test diffs mechanical-only across the whole branch; HC2/F2-11/generic-floor integrity at the final head. These wait for claim(rcust): M1.

Interim pre-review of frozen P5 (branch @ 68954be) — @2026-06-10T19:06Z, cold from fresh host clone

Builder landed P5 (customization manifest) and moved to P6, so P5 is frozen. Pre-reviewed it cold. No blocking defect; one secret-SURFACE observation raised (heads-up to Builder, NOT a VETO, NOT an M1 secret-leak failure). NOT an M1 verdict.

Cold acceptance (fresh git clone on cc-ci host at 68954be, my own checkout):

  • cc-ci-run -m pytest tests/unit -q → 191 passed (exact match to claim).
  • nix develop .#lint --command scripts/lint.sh → lint: PASS.

Primary adversarial target — SECRET LEAKAGE via the new manifest surface (D-gate: published logs + dashboard contain NO secrets, incl. generated app passwords):

  1. Generated/runtime secrets — NOT exposed (gate holds). manifest.build collects only: meta_non_default (static recipe_meta), hook NAMES (pre-ops/install_steps.sh/compose.ccci.yml), overlay FILENAMES, custom-test COUNTS, and env-override KEY names (printed KEY=1, value never rendered). It never touches deps (client_secret), op_state, abra-generated app passwords, or any env VALUE. The cardinal concern — generated app passwords on the dashboard — is structurally absent from this surface.
  2. Cold all-recipes sweep. Built+rendered the manifest for all 21 recipes on the host; grepped the rendered blocks AND the results.json customization payload for secret/password/token/key/ credential and for any 32+ char high-entropy string. The ONLY hit, across every recipe, is plausible's EXTRA_ENV.SECRET_KEY_BASE = "ccciplausibletestkeybase64charsexactlyforCIephemeral4567890123".
  3. OBSERVATION (not a leak): that value is a HARDCODED, committed, PUBLIC dummy CI constant (tests/plausible/recipe_meta.py, in the open-source repo) — not a generated or real secret. meta_non_default dumps EXTRA_ENV literal dicts verbatim into the log AND results.json (→ dashboard), so a field literally named SECRET_KEY_BASE with a value now appears on the dashboard. No real secret is exposed (it's public), so this is NOT a D-gate failure and does NOT block P5. BUT it's a standing surface: (a) a dashboard secret-scan gets a true-positive-shaped hit on a public dummy (noise that could mask a real leak), and (b) if any recipe ever set a real secret-ish literal in a meta dict, the manifest would surface it unredacted. Flagged to Builder via BUILDER-INBOX as a heads-up to consider redacting values of sensitive-named meta keys before M1. Will re-examine on the real dashboard at the M1 cold-verify.
  4. HC2-honoring — confirmed. Manifest routes ALL repo-local reads through discovery._gated (ops.py loop direct; install_steps/resolve_overlay_op/custom_tests each call _gated internally). An unapproved repo-local recipe contributes nothing to the manifest.
  5. Pure presentation — holds. build() only reads files/env and returns a dict; render() formats a string. Called at run_recipe_ci.py:889-890 (print) + embedded at :1261 into results; no state mutation, no verdict influence. _jsonable renders callables as '<hook>' (so a callable EXTRA_ENV/READY_PROBE never leaks closure internals) and tuples→lists for JSON.

Net: P1–P5 all clean under cold adversarial probing; every phase claim (unit count + lint) reproduced cold. No findings filed; no VETO. One non-blocking secret-surface heads-up sent.

Still UNVERIFIED for M1: P6 (docs); pytest tests/concurrency -q cold; R2 end-to-end via the real orchestrator screenshot path; final coverage re-diff on the COMPLETE branch (all 21 recipes, effective customization unchanged); recipe-test diffs mechanical-only across the whole branch; HC2/F2-11/generic-floor integrity at final head; AND — at the M1 dashboard check — confirm the SECRET_KEY_BASE-named field on the real dashboard is the accepted public dummy (or redacted). These wait for claim(rcust): M1.

M1 — implementation verified: PASS @2026-06-10T19:27Z (branch restructure/recipe-custom @ 858e0f5)

Cold-verified from TWO fresh clones on the cc-ci host (NEW=858e0f5, OLD=main pre-restructure; merge-base 49fb818 confirmed → main..858e0f5 is exactly P1–P6). Verdict formed from the phase plan (SSOT), the code/git history, the STATUS verification facts, and my own cold re-runs — NOT from JOURNAL rationale (isolation discipline; I did not need to consult JOURNAL).

All M1 Definition-of-Done items PASS:

  1. Cold test suites — match claim exactly. Fresh clone @858e0f5: cc-ci-run -m pytest tests/unit -q → 192 passed; tests/concurrency -q → 23 passed (untouched by this plan, proven); nix develop .#lint --command scripts/lint.sh → lint: PASS.

  2. Coverage diff (cardinal risk) — 0 REAL deltas / 21 recipes. Wrote throwaway extractors that resolve EVERY recipe's effective customization in BOTH worlds — OLD via the legacy loaders (_load_meta + lifecycle._recipe_extra_env + deps.declared_deps + _recipe_meta_flag), NEW via meta.load() + meta.extra_env/upgrade_extra_env — for the common keys (HEALTH_*, timeouts, DEPS, EXTRA_ENV resolved at a fixed domain, UPGRADE_EXTRA_ENV, BACKUP_CAPABLE, EXPECTED_NA, UPGRADE_BASE_VERSION, READY_PROBE/BACKUP_VERIFY presence). Diff = 0 behavioral deltas; the only raw diffs were 20× UPGRADE_EXTRA_ENV: None→{} (unset default representation, behaviorally identical) and mumble (most-customized: callable EXTRA_ENV→dict, UPGRADE_EXTRA_ENV, READY_PROBE) is byte-identical old↔new. Deleted keys accounted for (no silent loss): SKIP_GENERIC (0 recipe users); CHAOS_BASE_DEPLOY → overlay-presence (discourse+ghost, exactly the two shipping compose.ccci.yml — perfect 1:1, no change either direction); OIDC_AT_INSTALL → install-time made universal (drive+meet were already install-time). lasuite-docs declared DEPS but NOT OIDC_AT_INSTALL → OLD post-install, NEW install-time: an INTENTIONAL P2b consolidation, not a drop — flagged below for M2 validation.

  3. Assertion weakening (VETO-class) — NONE. Full branch diff over all recipe test files (excl. harness unit/concurrency/regression): 18 removed asserts, 18 added. After mechanical normalization (domain→ctx.domain, deps_creds→deps, MAX_USERS→_MAX_USERS, whitespace) the removed and added assert sets are IDENTICAL — zero unmatched in either direction. Every change is a pure signature/fixture/constant rename; no expected value altered, no assert deleted. Spot-confirmed discourse/ghost _psql(domain,…ci_marker…) in (…) → ctx.domain only (expected tuple + SQL byte-identical). No VETO.

  4. Deleted-code fallout — clean. No dangling LIVE refs to any of the 13 deleted symbols (_recipe_meta/_load_meta/_recipe_extra_env/_recipe_meta_flag/declared_deps/ is_canonical_enrolled/OIDC_AT_INSTALL/CHAOS_BASE_DEPLOY/SKIP_GENERIC/setup_custom_tests/ deps_apps/deps_creds/deployed_app). Only residue: stale DOC/comment mentions of OIDC_AT_INSTALL + setup_custom_tests.sh in PARITY.md files (non-blocking P6 cosmetic nit).

  5. Validation gaps — closed. Cold-probed meta.load() with synthetic bad metas: typo'd key, str-on-int, bool-as-int, callable-on-data-key, legacy hook sig READY_PROBE(domain), and unknown key ALL → MetaError (clear, names the offending file/key). Clean + underscore-private-helper metas load fine (no false positives). No silent pass.

  6. R2 fixed end-to-end. Cold proof through the REAL load path: a recipe declaring def SCREENSHOT(page, ctx) is surfaced by meta.load() and resolved callable by screenshot._load_screenshot_hook (old L1 allowlist dropped it — now arrives); orchestrator wires it run_recipe_ci.py:1029 capture(…, recipe_meta=meta) → hook(page, hook_ctx(domain, meta)). Absent recipe → None (default landing-page path). Legacy SCREENSHOT(page, domain, meta) sig rejected at load.

  7. HC2 / F2-11 / generic-floor integrity — preserved. Cold-probed discovery.custom_tests + install_steps: UNAPPROVED repo-local → [] / None (default-deny holds); APPROVED → surfaced. sso_dep_unverified (F2-11) logic UNCHANGED (only a comment edited) — a deps-not-ready run that skips ≥1 requires_deps test still suppresses the green signal. Generic floor _skip_generic default = run (additive); opt-out now env-only (same env vars as before; the 0-user meta key removed) and surfaced LOUDLY in CI + flagged !! in the manifest — strictly stronger, never silent.

  8. (Bonus) P5 secret-surface heads-up RESOLVED + verified. The Builder landed 858e0f5 redacting secret-named meta values in the manifest (my P5 BUILDER-INBOX ask). Cold-verified: plausible.EXTRA_ENV.SECRET_KEY_BASE → <redacted> in BOTH the log block and results.json; recursive into nested dict keys; word-segment (^|_)KEY(_|$) regex avoids over-match (KEYCLOAK_* passes). All-21-recipe sweep: exactly 1 redaction, ZERO over-redaction, ZERO under-redaction (no secret-shaped value remains). Regression test test_manifest_redacts_sensitive_named_values present.

Verdict: M1 PASS. No findings filed, no VETO.

This does NOT clear ## DONE. Per the phase DoD, DONE requires a fresh Adversary PASS for BOTH M1 and M2. M2 (merged-main real-CI regression sweep vs the committed baseline matrix) is still unverified. M2 watch-items I will specifically re-check from run logs:

  • lasuite-docs OIDC is now install-time (post→install change above) — must pass a real run with OIDC wired at install (skip-count 0 on its requires_deps tests).
  • the customization spot-checks the plan §M2.4 enumerates (mumble READY_PROBE tcp lines, cryptpad SANDBOX_DOMAIN, ghost/discourse BACKUP_VERIFY + overlay copy + auto-chaos base deploy, lasuite-* deps provisioning + OIDC tests ran, immich ops.py seeds, manifest block present in every log, screenshot.png where capture succeeded).
  • canary suite (RED canaries still caught at intended tier) + per-recipe level == baseline matrix.
  • zero leaked apps after teardown.