diff --git a/docs/changelog-20260709-adversarial-review-fixes.md b/docs/changelog-20260709-adversarial-review-fixes.md new file mode 100644 index 0000000..6d1d461 --- /dev/null +++ b/docs/changelog-20260709-adversarial-review-fixes.md @@ -0,0 +1,110 @@ +# Changelog 2026-07-09 -- Adversarial review pass: 3 findings fixed (DOCFIX-160) + +No live infrastructure touched. Operator's standing instruction ("check +back over current work, review for errors/assumptions") applied to +everything built this evening: `scripts/lib-net.sh`/`lib-hosts.sh`'s +selector functions, `netbox/dc-dc-prefixes-import.py`, and all seven +Stage 1-7 runbooks. Ran a dedicated adversarial-review subagent (fresh +eyes, no authoring context) instructed to find real defects, not summarize +-- it read every file in full, cross-checked claims against +`design-decisions.md`/`bundle.yaml`/`opentofu/*.tf` directly, and ran both +new test harnesses live rather than trusting their reported pass counts. + +## Items + +### 1. DOCFIX-160a -- dead-code bug in `tests/dc-dc-prefixes-import/test_logic.py` +FILE: `tests/dc-dc-prefixes-import/test_logic.py`. + +WHAT: `expect_ok()`'s `try: return fn(...) / except SystemExit: ... / else: +ok(label)` structure has a real Python control-flow bug -- a `return` +statement inside a `try` block causes the function to exit immediately, +so the `else` clause (which only runs when the `try` block completes +WITHOUT a `return`/exception) is UNREACHABLE dead code whenever `fn()` +succeeds. Verified empirically (`python3 -c` reproduction): the `else` +branch's `print`/`ok()` call never executes on the success path. + +CONCRETE IMPACT: three happy-path assertions -- +`"ula48 valid /48 within fc00::/7"`, `"gua valid /40 within +2602:f3e2::/32"`, `"dc2 supernet valid /19 no overlap"` -- were silently +never counted as PASS *or* FAIL. The harness's "ALL PASS (37 checks)" +banner was true for the 37 checks that DID run, but overstated what was +actually verified: the safety property (rejecting bad input) was still +exercised correctly (that path lives in `except`, unaffected by the bug), +but "valid input is accepted, not spuriously rejected" was unverified and +uncounted. This "37/37" figure was quoted in `docs/dc-dc-deployment- +workflow.md`, `docs/changelog-20260709-netbox-dc-dc-pipeline.md`, and +`docs/session-ledger.md` -- all now stale by 3 checks (see item 3 below). + +FIX: restructured to assign the result inside `try`, call `ok(label)` +unconditionally after the `except` block (which itself still returns +early on failure), and return the result: +```python +def expect_ok(label, fn, *a, **kw): + try: + result = fn(*a, **kw) + except SystemExit as exc: + no(label, f"unexpectedly raised SystemExit({exc})") + return None + ok(label) + return result +``` + +VERIFIED: re-ran the harness -- the three previously-silent labels now +print `PASS`, and the total moved from 37 to **40 checks, all passing**. + +### 2. DOCFIX-160b -- self-contradictory comment in `netbox/dc-dc-prefixes-import.py` +FILE: `netbox/dc-dc-prefixes-import.py` (lines 165-168). + +WHAT: two adjacent comments about the same arithmetic fact contradicted +each other -- one claimed a /19 "exactly fits six /22s with zero slack," +the other correctly stated "a /19 = exactly 8x /22." 2^(22-19) = 8, so a +/19 holds eight /22s (six used, two spare), not six with no slack. No +functional bug -- the code only requires prefixlen <= 19 and slices the +first six subnets, which was always correct -- but a genuine, verifiable +self-contradiction a few lines apart, exactly the class of thing an +adversarial pass should catch. + +FIX: corrected the wrong comment to match the arithmetic the other +comment (and the code itself) already had right. + +### 3. DOCFIX-160c -- stale self-referential text in Stage 4 runbook +FILE: `runbooks/dc-dc-phase3-maas-enlist-deploy.md`. + +WHAT: written mid-session when Stage 3's runbook didn't exist yet, two +spots said so explicitly ("Stage 3's own runbook does not exist yet as of +this writing... state NOT STARTED"). By the time the full session +finished, Stage 3 (and Stage 4 itself) were both written and reflected as +such in `docs/dc-dc-deployment-workflow.md` -- these two spots were never +reconciled against that final state. Not misleading in practice (the +runbook already hedged with "if it has been written and run by the time +you execute this..."), but stale phrasing left behind. + +FIX: reworded both spots to state Stage 3's runbook is now written but not +yet EXECUTED, and to check its actual execution state (not just its +existence) before proceeding. + +VERIFIED (all three items): `bash scripts/repo-lint.sh` 0 fail, 1 +documented legacy warn. `bash tests/dc-dc-prefixes-import/run-tests.sh`: +40/40 PASS (up from 37, item 1's fix). `bash tests/dc-selector/run- +tests.sh`: 21/21 PASS (unaffected, re-run to confirm no regression from +touching a neighboring file in the same review pass). + +**What the review found nothing wrong with, despite genuinely trying:** +the `$DC` selector asymmetry description (identical and correct across +every file that mentions it); the ULA/GUA carve arithmetic (hand-verified +deterministic, no cross-DC collision); Stage 6's `bundle.yaml` claims +(confirmed byte-for-byte); 3+ direct D-NNN quotes spot-checked verbatim; +DOCFIX numbering and D-100..D-110 ratification consistency across every +changelog/tracker/ledger entry; every `opentofu/main.tf`/`variables.tf` +claim in the Stage 1/3 runbooks. No inferred/invented literal value was +found anywhere in the reviewed material. + +REVERT: not applicable -- all three fixes are themselves corrections to +today's own deliverables, reverting them would reintroduce the bugs. + +## Next actionable step + +Adversarial review pass complete (this session's task list item). Final +consolidation (workflow doc cross-check, gap register, companion artifact, +changelog roll-up, session ledger) is the last remaining item before this +evening's work is fully wrapped for the operator's return in the morning. diff --git a/docs/dc-dc-deployment-workflow.md b/docs/dc-dc-deployment-workflow.md index 6d9cca1..6a5d996 100644 --- a/docs/dc-dc-deployment-workflow.md +++ b/docs/dc-dc-deployment-workflow.md @@ -232,7 +232,7 @@ hardcoded per D-101; DC2 via required `DC2_V4_SUPERNET`, no default) plus the D-101 family-matrix IPv6 legs (ULA/GUA), carved from two REQUIRED env vars (`ORG_ULA_48`, `DC_GUA_PREFIX`) with no defaults -- fails loud - rather than inventing either. 37/37 tests (`tests/dc-dc-prefixes- + rather than inventing either. 40/40 tests (`tests/dc-dc-prefixes- import/`, fake-NetBox end-to-end). UNVALIDATED against a real NetBox (no instance reachable this session). The literals themselves (org ULA /48, per-DC GUA carve, DC2 supernet) still don't exist -- that operator/NetBox @@ -376,5 +376,5 @@ | `opentofu/` (DC2 planes) | NOT DONE -- deliberately deferred pending NetBox CIDR assignment, see `opentofu/README.md` | | Reusable as-is, repo-agnostic (2026-07-09 sweep) | `bundle.yaml`, `phase-01..08` VR0 DC0 runbooks (Stage 5/7 template, once DC-parameterized -- gap #1), `preflight.sh`, `cloud-assert.sh`, `repo-lint.sh`, `run-logged.sh`, `ledger-scan.sh` | | `lib-net.sh` / `lib-hosts.sh` | `$DC` selector mechanism ADDED 2026-07-09 (DOCFIX-151, gap #1 CLOSED) -- `lib_net_select_dc()`/`lib_hosts_select_dc()`, backward compatible, 21/21 tests; Stage 5 runbook still needs to call it | -| NetBox DC-DC pipeline (`netbox/dc-dc-prefixes-import.py`) | MECHANISM BUILT 2026-07-09 (DOCFIX-152, gap #3 tooling half CLOSED) -- 37/37 fake-NetBox tests, UNVALIDATED live; real literals (ULA /48, GUA carve, DC2 supernet) still unassigned | +| NetBox DC-DC pipeline (`netbox/dc-dc-prefixes-import.py`) | MECHANISM BUILT 2026-07-09 (DOCFIX-152, gap #3 tooling half CLOSED) -- 40/40 fake-NetBox tests (DOCFIX-160 fixed a dead-assertion test bug that had silently left 3 of these uncounted), UNVALIDATED live; real literals (ULA /48, GUA carve, DC2 supernet) still unassigned | | Environment/skill docs (`environment.md`, README.md) | Still describe VR0 DC0/v1 single-DC scope wholesale -- flagged, not yet rewritten for VR1; not a blocker for Stage 0 but will need doing before/during Stage 1 | diff --git a/docs/session-ledger.md b/docs/session-ledger.md index fbd1ea2..5efff75 100644 --- a/docs/session-ledger.md +++ b/docs/session-ledger.md @@ -21,8 +21,8 @@ ## Machine-derived (re-seed from `scripts/ledger-scan.sh`; do not hand-edit) -_As of 2026-07-09 scan (post-DOCFIX-159, ALL seven Stage 1-7 runbooks -written); re-run the scan to refresh:_ +_As of 2026-07-09 scan (post-DOCFIX-160, adversarial review pass fixes); +re-run the scan to refresh:_ - **PROPOSED / OPEN decisions:** D-068 (Vault substrate hardening, Roosevelt -- 1.16 ruled out per amendment; off-EOL path OPEN), D-071 (routine update cadence + @@ -37,8 +37,8 @@ - **OPEN security rows:** SEC-001 (rotate at v1 close, re-ruled 2026-07-06), SEC-003 (custodians + second-person unseal -- DEFERRED; keeps d011-06 MANUAL), SEC-004 (flip repo to private at v1 close -- deferral reaffirmed). -- **Next-free numbers:** D = 111, DOCFIX = 160, BUNDLEFIX = 012. - (Seeded from the 2026-07-09 scan, post DOCFIX-159.) +- **Next-free numbers:** D = 111, DOCFIX = 161, BUNDLEFIX = 012. + (Seeded from the 2026-07-09 scan, post DOCFIX-160.) --- @@ -962,7 +962,10 @@ blocks is flagged in the script's own docstring as a proposed, unratified convention pending operator review. New harness `tests/dc-dc-prefixes-import/` (a fake in-memory NetBox client): 37/37 - PASS. Caught and fixed two real authoring bugs: (1) example GUA addresses + PASS at the time (later corrected to 40/40 by DOCFIX-160 -- a dead-code + bug in the harness itself had left 3 happy-path assertions silently + uncounted; see that entry below). Caught and fixed two real authoring + bugs at delivery time: (1) example GUA addresses in the first draft had host bits set beyond the /40 boundary, caught by Python's own `ipaddress.ip_network(strict=True)`; (2) the test's own "count CREATED lines" assertion was inflated by the script's unrelated @@ -1060,6 +1063,35 @@ this whole runbook-authoring pass surfaced (Office1-local network missing, IPv6 family-matrix overlay missing, D-108 bundle gaps, Stage-4 MAAS/multi-rack gaps) -- see `docs/dc-dc-deployment-workflow.md`. +- **DONE -- Adversarial review pass across the whole evening's work + (changelog `docs/changelog-20260709-adversarial-review-fixes.md`, + DOCFIX-160):** ran a dedicated fresh-eyes review subagent (no authoring + context) across `lib-net.sh`/`lib-hosts.sh`'s new selectors, + `netbox/dc-dc-prefixes-import.py`, and all seven Stage 1-7 runbooks, + instructed to find real defects and cross-check every claim against + ground truth directly (not trust the runbooks' own self-report) -- + including running both new test harnesses live. Found and fixed 3 real + issues: (1) a genuine Python control-flow bug in `tests/dc-dc-prefixes- + import/test_logic.py`'s `expect_ok()` -- a `return` inside `try` made + the `else` clause unreachable dead code, silently leaving 3 happy-path + assertions uncounted (37/37 was true for what ran, but overstated what + was verified; now 40/40 with the fix, independently re-verified via a + live `python3 -c` reproduction before trusting the agent's claim); (2) a + self-contradictory pair of adjacent comments in `dc-dc-prefixes- + import.py` about /19-vs-/22 arithmetic (no functional bug, just wrong + prose); (3) stale "as of this writing" self-references in the Stage 4 + runbook, written before Stage 3 existed and never reconciled once it + did. Propagated the corrected 40/40 count into `docs/dc-dc-deployment- + workflow.md` and this ledger's own earlier entry (left the original + delivery changelog un-edited as historical record of what was believed + at the time). The review explicitly found NOTHING wrong with: the $DC + selector asymmetry description (consistent everywhere), the hand- + verified ULA/GUA carve arithmetic, Stage 6's `bundle.yaml` claims + (confirmed byte-for-byte), 3+ direct D-NNN quotes (verbatim-accurate), + and DOCFIX/D-NNN numbering consistency across every file -- and found no + inferred/invented literal value anywhere in the reviewed material. + repo-lint 0 fail/1 documented warn; both affected test suites re-run + clean (40/40 and 21/21) after the fixes. diff --git a/netbox/dc-dc-prefixes-import.py b/netbox/dc-dc-prefixes-import.py index 613e052..9bf69db 100644 --- a/netbox/dc-dc-prefixes-import.py +++ b/netbox/dc-dc-prefixes-import.py @@ -163,8 +163,8 @@ # DC2 v4: NO default. Must come from DC2_V4_SUPERNET (env var), carved into # six sequential /22s in PLANE_ORDER, exactly mirroring DC1's /22-per-plane # structure. The supernet must be large enough to hold six non-overlapping -# /22s (a /19 exactly fits six /22s with zero slack; anything larger works -# with room to spare, anything smaller FAILS LOUD). +# /22s (a /19 holds exactly eight /22s -- six used, two spare; anything +# smaller than /19 FAILS LOUD). DC2_MIN_SUPERNET_PREFIXLEN = 19 # a /19 = exactly 8x /22, room for the six planes diff --git a/runbooks/dc-dc-phase3-maas-enlist-deploy.md b/runbooks/dc-dc-phase3-maas-enlist-deploy.md index 16efa97..d23198e 100644 --- a/runbooks/dc-dc-phase3-maas-enlist-deploy.md +++ b/runbooks/dc-dc-phase3-maas-enlist-deploy.md @@ -14,9 +14,9 @@ (OpenTofu/MAAS/Juju boundary: "MAAS: enlist (discover) / commission / deploy / power / release of node VMs. Does not compose. One region (Office1) + one rack per DC (virsh VM-host)."); `docs/dc-dc-deployment-workflow.md` Stage 4 -row (State: NOT STARTED as of this writing; "Reuse vs new: PARTIAL reuse... -the per-DC multi-rack registration is new, the deploy mechanics are -precedent"); `docs/design-decisions.md` **D-103** (lifecycle seam: MAAS owns +row ("Reuse vs new: PARTIAL reuse... the per-DC multi-rack registration is +new, the deploy mechanics are precedent"); `docs/design-decisions.md` +**D-103** (lifecycle seam: MAAS owns commission/deploy/power/release, does NOT compose), **D-100** (br-ex / provider-NIC raw discipline, Pattern A), **D-107** (per-DC airgap mirror + edge NTP). `runbooks/phase-00-teardown-maas-reset.md` Steps 5-7 are the @@ -31,9 +31,10 @@ ## Precondition -- Stage 3's gate must already be green (READ-ONLY) -Stage 3's own runbook does not exist yet as of this writing (workflow doc: -"Runbook not started"; state NOT STARTED) -- if it has been written and run -by the time you execute this, confirm its EXIT GATE against +Stage 3's own runbook (`runbooks/dc-dc-phase2-tofu-dc-substrate.md`) is +written but NOT YET EXECUTED as of this writing -- confirm it has actually +been RUN (not merely authored) for `$DC` before proceeding, by checking its +own EXIT GATE against `docs/dc-dc-buildout-design.md` Section 4 Phase 2 gate text: "MAAS rack controller per DC up; MAAS discovers the OpenTofu-created node VMs; edge routing + simulated ISP uplink up; netem parameters applied and measured." diff --git a/tests/dc-dc-prefixes-import/test_logic.py b/tests/dc-dc-prefixes-import/test_logic.py index 2d8ac26..13821a5 100644 --- a/tests/dc-dc-prefixes-import/test_logic.py +++ b/tests/dc-dc-prefixes-import/test_logic.py @@ -63,12 +63,12 @@ def expect_ok(label: str, fn, *a, **kw): try: - return fn(*a, **kw) + result = fn(*a, **kw) except SystemExit as exc: no(label, f"unexpectedly raised SystemExit({exc})") return None - else: - ok(label) + ok(label) + return result def load_target():