ADVERSARIAL REVIEW -- D-139 STEP 6 TOOL ======================================= Date : 2026-08-02 Reviewer : adversarial review pass (subagent), jumphost vcloud Targets : netbox/d139-step6-vip-rehome.py (untracked, written 2026-08-02) tests/d139-step6-vip-rehome/run-tests.sh (untracked, written 2026-08-02) Governing : docs/design-decisions.md - "ORDERING RULING 2026-08-02 -- D-139 step 6 executes IN FULL before the Stage-5 deploy" -- operator: "Full step 6 first, then deploy" - "AMENDMENT 2026-08-02 -- D-139 step 6: 'retire' means DEPRECATE, and nothing is deleted" -- operator: "Deprecate both, delete nothing" - "CORRECTION NOTE 2026-08-01 -- D-139: the EXECUTION list was defective" Apex : http://10.10.1.10:8000 (office1-netbox, the WORKING VR1 apex, DOCFIX-195) VERDICT: FIX FIRST Do NOT run --commit as the tool stands. Two MEASURED defects are reachable without any operator error beyond choosing the other valid value of a required flag, and the harness does not test the one property both operator rulings name as load-bearing. The tool's core mapping logic is CORRECT and its dry run reproduces the ruling's measured 26 / 26 / 9 exactly. The defects are missing PRECONDITIONS and missing COVERAGE, not a wrong carve. METHOD AND HONESTY BOUNDARY MEASURED = observed from a command run this session, output reproduced below. REASONED = derived from reading the source; explicitly labelled where used. Nothing in the DEFECTS section is REASONED-only; every one carries a run. --commit was NEVER executed. All live apex access was read-only (GET). Mutation experiments were run against COPIES under /home/jessea123/.claude/jobs/015c103e/tmp/mut/ . No repo file was edited at any point. ------------------------------------------------------------------------------- BASELINE -- what the tool does today, MEASURED ------------------------------------------------------------------------------- $ . ~/vr1-office1-creds/vr1-netbox-sandbox.env $ python3 netbox/d139-step6-vip-rehome.py --dc vr1-dc0 (DRY RUN, rc=0) D-139 STEP 6 -- vr1-dc0 -- apex http://10.10.1.10:8000 CREATE 26 | ALREADY 0 | DEPRECATE-ADDR 26 | DEPRECATE-PFX 9 The 26 creates are 2602:f3e2:f02:20::50-::62 (metal-admin) and 2602:f3e2:f02:21::50-::62 (metal-internal), status=reserved, descriptions verbatim. This matches the ORDERING RULING's measured state exactly. Host octets are preserved, the ULA->GUA hextet mapping is right, and the /60 parents are deprecated but hold no addresses. The offline harness reports PASS=14 FAIL=0. Independent read-only enumeration of the apex (152 prefixes, 160 addresses, 27 ranges) confirms, MEASURED: - 52 ULA v6 addresses exist: 26 in dc0 (fd50:...:220,:221), 26 in dc1 (:320,:321). - All 26 dc0 ULA VIP records carry status=reserved. VIP_STATUS="reserved" is correct. - Zero ip-ranges inside the retired ULA /48. Zero duplicate prefix CIDRs apex-wide. - No provider-public ULA /64 exists, so the ambiguity REFUSE (:10 vs :11) is not reachable on this apex today. It is a live-safe design, not a live-blocking one. CHECKED AND CLEAN (a gap I suspected and disproved): overlays/vr1-dc0-vips.yaml declares 39 GUA VIP legs -- f02:11 x13, f02:20 x13, f02:21 x13 -- and ZERO fd50: legs. Step 6 only creates 26. The missing 13 are NOT a gap: 2602:f3e2:f02:11::50-::62 ALREADY EXIST in the apex as ip-address records ("VIP provider-public v6 (vr1-dc0) -- octet mirror ::NN"). So 13 existing + 26 created = the full 39, and after step 6 the apex records every VIP the overlay declares. No action needed. ------------------------------------------------------------------------------- DEFECTS, RANKED BY CONSEQUENCE ------------------------------------------------------------------------------- DEF-1 [CRITICAL -- defect MEASURED, consequence REASONED] --dc vr1-dc1 creates 26 ORPHAN addresses and deprecates dc1's only valid records. No precondition check that step 1 ran for the DC being operated on. --dc accepts choices=sorted(C.DC_GUA), i.e. BOTH vr1-dc0 and vr1-dc1 (d139-step6-vip-rehome.py:152). vr1-dc1 is one keystroke from vr1-dc0. MEASURED, the apex's vr1-dc1 GUA carve is INCOMPLETE -- only 4 rows exist under 2602:f3e2:f03::/48, all provider-public: 2602:f3e2:f03::/48 vr1-dc1 dc (container) 2602:f3e2:f03:10::/60 vr1-dc1 provider-public 2602:f3e2:f03:10::/64 vr1-dc1 provider-public 2602:f3e2:f03:11::/64 vr1-dc1 provider-public There is NO 2602:f3e2:f03:20::/64 and NO 2602:f3e2:f03:21::/64. MEASURED, the tool nevertheless plans (dry run, rc=0, no warning of any kind): D-139 STEP 6 -- vr1-dc1 -- apex http://10.10.1.10:8000 CREATE 26 | ALREADY 0 | DEPRECATE-ADDR 26 | DEPRECATE-PFX 9 ... 2602:f3e2:f03:20::50/64 .. :20::62, 2602:f3e2:f03:21::50 .. :21::62 DEPRECATE prefixes: fd50:840e:74e2:320::/60, :320::/64, :321::/64, :330::/60, :330::/64, :340::/60, :340::/64, :350::/60, :350::/64 With --commit this would write 26 ip-addresses into /64s that DO NOT EXIST as prefix rows, then mark dc1's 26 authoritative ULA addresses and 9 ULA prefixes deprecated, and exit 0. dc1's VIPs would then be recorded ONLY as orphans with no containing prefix, while every row that currently describes them reads unusable. This is exactly the orphaning hazard DEFECT 3 of the 2026-08-01 CORRECTION NOTE names, and exactly what the ordering ruling exists to prevent -- reached not by a race or a partial failure but by the tool's own documented interface. Note the asymmetry that makes this easy to miss: gua_net_for_role() computes the target /64 ARITHMETICALLY from C.DC_GUA and C.CARVE. It never asks the apex whether that /64 exists. For vr1-dc0 all 16 target prefixes happen to exist (step 1 applied), so dc0's dry run looks perfect and hides the missing check entirely. Compare the imported tool: d139-gua-carve.py checks its preconditions before writing (exactly one site with the slug, line 177-180; every CARVE role exists in the apex, line 183-186, "refusing to half-carve a DC"). d139-step6-vip-rehome.py checks none. FIX (suggested, operator's call): before planning, require that every GUA /64 the plan targets EXISTS as a prefix row scoped to --dc, and die() if not, naming step 1. Optionally also restrict --dc to vr1-dc0 for this window. DEF-2 [CRITICAL -- defect MEASURED, consequence REASONED] The SANDBOX_HOSTS apex-identity guard present in the imported tool is ABSENT from the new tool. It will write to whatever NETBOX_URL names. d139-gua-carve.py carries the guard: :25 SANDBOX_HOSTS = {"localhost", "127.0.0.1", "10.10.1.10"} :161 if host not in SANDBOX_HOSTS: :162 die("'%s' is not the VR1 working apex. office1-netbox (10.10.1.10) takes ALL VR1 reads and writes; netbox.baldurkeep.com is the v1 REFERENCE and stays untouched (DOCFIX-195)." % host) d139-step6-vip-rehome.py: grep -c SANDBOX_HOSTS -> 0. No urlsplit, no hostname check. The guard lives inside d139-gua-carve.py's main(), which the import never executes, so subclassing C.NB inherits the transport but NOT the safety posture. MEASURED differential against a non-sandbox host (127.0.0.9:1, unreachable, dummy token -- nothing real was contacted): carve: REFUSE: '127.0.0.9' is not the VR1 working apex. ... (DOCFIX-195) rc=2 step6: REFUSE: apex unreachable or rejecting: GET /ipam/prefixes/?limit=500 -> rc=2 Both exit 2 here only because the host is unreachable. The carve tool refused on IDENTITY, before any network call; the step-6 tool refused on REACHABILITY. Against a REACHABLE non-sandbox apex -- netbox.baldurkeep.com, the v1 REFERENCE that DOCFIX-195 says stays untouched -- the step-6 tool would proceed to plan and, with --commit, write. One wrong `.` of an env file is the whole distance. This defect is a REGRESSION against the imported module's own stated posture, which is the strongest argument for fixing it: the reviewer of step 1 already decided this guard was required for a WRITE tool against this apex. FIX: lift the same guard (reuse C.SANDBOX_HOSTS, do not retype the set). DEF-3 [HIGH, MEASURED] SILENT UNDER-COUNT: a missing or mis-scoped ULA /64 prefix row silently halves the work and the run still exits 0. dep_addrs and creates are derived ONLY from addresses contained in a /64 prefix ROW that is scoped to --dc and inside the retired ULA /48 (plan(), lines 102-141). The addresses themselves are never counted independently, and no expected total is asserted anywhere. MEASURED offline against the pristine repo file, full 26-address fixture, with only the fd50:840e:74e2:221::/64 PREFIX row removed (its 13 addresses still present): D1 the :221::/64 prefix ROW is absent -> CREATE=13 DEPA=13 DEPP=8 ALREADY=0 -- no complaint, plan() returns D1b the :221::/64 row's scope is blank/name-only instead of the slug -> CREATE=13 DEPA=13 DEPP=8 ALREADY=0 -- same Thirteen live VIPs would be neither created nor deprecated, and the run would report success. D1b is not hypothetical shape-wise: scope_slug() itself exists because the live API returns scope.slug while repo dumps normalise scope.name, and its own docstring records that matching on name alone "found ZERO live". This is the same class as CORRECTION NOTE DEFECT 2 -- dc-node-v6-carve.py carving FOUR FEWER PLANES PER NODE and EXITING CLEAN -- and the same class as the MEMORY entry "instrument currency before negatives". The carve tool guards its half of this (build_retire(): "the apex returned ZERO prefixes scoped to %s ... refusing to call every plane 'missing' on that evidence"). The step-6 tool guards only the total-zero case (`if not ula_pfx`), not the partial case, which is the one that actually bites. FIX: assert a floor. The ruling records 26 / 26 / 9 as MEASURED; make the tool require it (e.g. --expect-creates / --expect-deprecate-addrs / --expect-deprecate-prefixes, or cross-check dep_addrs against the count of ALL v6 addresses inside the retired /48 that are NOT in another DC's scoped prefixes). Any tool that can do less than the ruling specifies and still exit 0 is not gated. DEF-4 [HIGH, MEASURED] The HARNESS does not test main() at all -- so the CREATE -> verify -> DEPRECATE ORDER, the one property both rulings name, is UNCOVERED. All of T1-T12 call plan(). Nothing calls or inspects main(). The harness header states "the tool's write path is CREATE -> verify -> DEPRECATE ... plan() is what decides all three sets, so it is where the risk lives" -- that sentence is the bug. plan() decides the SETS; main() decides the ORDER, and ORDER is what the ruling constrains: "CREATE FIRST, VERIFY, THEN DEPRECATE -- reversed, there would be an interval in which the apex marks a live VIP's only record unusable." MUTATION M1b, MEASURED. Into a COPY of the tool, immediately before the "PHASE 1: CREATE" block, inserted: for _x in dep_addrs: nb.patch("/ipam/ip-addresses/%d/" % _x["id"], {"status": DEPRECATED}) for _x in dep_prefixes: nb.patch("/ipam/prefixes/%d/" % _x["id"], {"status": DEPRECATED}) i.e. the tool now deprecates every ULA row BEFORE creating a single GUA record -- precisely the forbidden ordering. Harness result: RESULT: PASS=14 FAIL=0 ALL PASS The suite cannot see the defect it was written to prevent. FIX: test main() with a fake NB that RECORDS the call sequence, and assert that no PATCH is issued before every POST has been issued and read back. Also assert the failure branches: a POST that raises leaves zero PATCHes; a read-back that comes back short leaves zero PATCHes. ORDERING IN THE SOURCE IS, HOWEVER, CORRECT AS WRITTEN (read, not executed): phase 1 posts all creates; a RuntimeError returns 1 with "NOTHING WAS DEPRECATED"; phase 2 re-reads /ipam/ip-addresses/ and returns 1 on any missing create, again before any patch; phase 3 patches; phase 4 re-reads and verifies status. I found no path in the source where a ULA row is deprecated while its GUA counterpart does not exist, EXCEPT the vacuous case described in DEF-6 below. This is REASONED from source plus the MEASURED dry run; the --commit path was not executed, per the review constraint. DEF-5 [MEDIUM, MEASURED] T13, the case that names itself PROOF-OF-TEETH, is an assertion on ABSENCE and therefore passes vacuously on ANY error in its own snippet. T13 is `if grep -q "MUTANT_FIRST=" ; then bad ; else ok ; fi`. Any output that lacks that string satisfies it -- including a traceback. MUTATION M0, MEASURED. The tool file was replaced (in the COPY) with one line of non-Python. Result: FAIL T1 .. FAIL T12 (all report "Traceback (most recent call last):") PASS T13 PROOF-OF-TEETH: a wrong /48 changes the target, so T2 is a real assertion PASS T14 tool contains NO delete path RESULT: PASS=2 FAIL=12 A tool that does not even parse passes the teeth test. Standing rule, script-authoring reference: "prove each new assertion can FAIL *and* prove it RAN." FIX: make T13 assert POSITIVELY -- require a MUTANT_FIRST line to be present AND to differ from the correct target (two greps, not one negation). DEF-6 [MEDIUM, MEASURED offline; NOT live today; reachable AFTER a first --commit run] A pre-existing but DEPRECATED GUA record is accepted as ALREADY, and its ULA counterpart is then deprecated too -- leaving the VIP with no usable apex row. plan() builds `existing` as a set of bare IPs (line 113) and the phase-2 read-back likewise compares bare IPs only (lines 205-210). Neither consults status. MEASURED offline, pristine tool, full fixture plus one deprecated GUA record: D3 GUA counterpart exists but is DEPRECATED -> CREATE=25 DEPA=26 DEPP=9 ALREADY=1 ::50 in creates? False ULA ::50 in dep_addrs? True After commit BOTH the ULA and the GUA record for that VIP read deprecated. Not live now (there are zero deprecated addresses in the apex). It becomes reachable the moment step 6 has run once and anyone flips a GUA row to deprecated -- i.e. it is an IDEMPOTENCE hazard on re-run, not purely theoretical. It is also the only route I found by which a ULA row ends up deprecated without a usable GUA counterpart, so it bears directly on the brief's item 1. FIX: treat a non-reserved/non-active existing GUA record as a REFUSE, or PATCH it back to VIP_STATUS as part of the create phase, and include status in the read-back predicate. DEF-7 [MEDIUM, MEASURED] T14's delete-path assertion covers ONE file, not the surface the tool actually has. Item 6 of the brief, answered directly: AS WRITTEN TODAY there is NO delete path. Verified by reading both files -- the only HTTP methods reachable are GET (get_all), POST (post) and PATCH (NBW.patch). C.NB._req takes an arbitrary `method` string but no caller passes DELETE. That is a clean finding. The ASSERTION that keeps it that way, however, greps "$TOOL" only. MUTATION M2, MEASURED. A delete() method was added to the COPY of the IMPORTED module d139-gua-carve.py (whose NB class step 6 subclasses), leaving the step-6 file untouched: def delete(self, path): return self._req("DELETE", path) RESULT: PASS=14 FAIL=0 T14 stays green while the class the tool inherits from has grown a delete path. The ruling "Deprecate both, delete nothing" is repo-wide; the assertion is file-wide. FIX: grep both netbox/d139-step6-vip-rehome.py and netbox/d139-gua-carve.py, or assert on the method set reachable from NBW. DEF-8 [LOW, MEASURED offline; NOT live] An address inside a ULA /60 parent but outside its /64 is silently dropped while the /60 is deprecated anyway. plan() skips any prefix with prefixlen != 64 (line 117-118) -- correct, and necessary: MUTATION M6 (removing that guard) makes CREATE jump 26 -> 52 and T1/T6/T7 all fire. THE GUARD IS DOING MORE THAN PREVENTING A DOUBLE-COUNT, and this is worth stating because it changes how much weight it carries: fd50:840e:74e2:221::/64 (metal-internal) is CONTAINED IN fd50:840e:74e2:220::/60 (role metal-admin). With the guard removed, the /60 matches all 26 addresses and maps the 13 metal-INTERNAL VIPs onto 2602:f3e2:f02:20::/64, the metal-ADMIN GUA plane -- 26 + 13 + 13 = 52, exactly the measured figure. So that one `continue` is the only thing standing between the tool and a silent CROSS-PLANE MIS-MAP. It must never be relaxed. But dep_prefixes deprecates ALL in-scope ULA prefixes including the /60s. MEASURED, pristine tool, fixture plus one address at fd50:840e:74e2:225::7 (inside :220::/60, outside :220::/64): CREATE=26 DEPA=26 DEPP=9 stray created? False stray deprecated? False BUT its /60 IS deprecated: True NOT LIVE: all 52 ULA addresses in the apex sit inside a /64. Latent only. DEF-9 [LOW, MEASURED offline; NOT live] Duplicate prefix ROWS for the same CIDR produce duplicate CREATE payloads. NetBox permits two prefix rows with the same CIDR. plan() iterates rows, not CIDRs, and the `already` check is against the pre-read `existing` set, never against creates itself. MEASURED, pristine tool, fixture plus a second row for :220::/64: CREATE=39 DEPA=39 DEPP=10 duplicate CREATE payloads: 13 (e.g. 2602:f3e2:f02:20::50/64) NOT LIVE: zero duplicate CIDRs measured apex-wide. Theoretical. DEF-10 [LOW, REASONED] ip_network.subnet_of() is true for a network against ITSELF, so if fd50:840e:74e2::/48 ever exists as a prefix ROW scoped to a DC it would be deprecated by this tool -- the org aggregate d139-gua-carve.py:48 calls "still a valid org aggregate". MEASURED: no such row exists today (the 18 in-/48 rows are all /60 or /64). Latent only; stated because the /48 is explicitly still-valid per the carve tool. DEF-11 [COSMETIC, MEASURED] tests/.../run-tests.sh defines run_case() at line 23-25 and never calls it. Dead code in a harness whose job is to be trusted. ------------------------------------------------------------------------------- ASSERTIONS THAT SURVIVED MUTATION ------------------------------------------------------------------------------- Mutation battery, each applied to a COPY, harness re-run, restored between runs: M0 tool replaced with unparseable text ..... T13, T14 PASS (see DEF-5) M1b deprecate loops moved before creates .... ALL 14 PASS (see DEF-4) M2 delete() added to imported carve module.. ALL 14 PASS (see DEF-7) M3 VIP_STATUS reserved -> active ........... T4 fires M4 ALREADY dedup removed ................... T6 fires M5 ambiguity REFUSE -> pick first hextet ... T10 fires M6 /60-parent skip removed ................. T1, T6, T7 fire M7 dep_prefixes forced empty ............... T1, T7, T12 fire M8 already-deprecated filter removed ....... T7 fires CONCLUSION: T1, T4, T6, T7, T10, T12 have real teeth against plan(). T2/T3 are proven by T13's construction (M6/M7 also move them indirectly). T13 and T14 are the two that survive everything (DEF-5, DEF-7), and NO assertion at all covers main() (DEF-4). CASE COUNT: claimed T1-T14; measured PASS=14 FAIL=0; count and labels agree. The suite's cases all RAN. (Standing rule "assert the case count moved" -- satisfied.) Note on coupling: T1-T5 all grep one shared $out from a single python run. A crash in that run turns five cases red together, which is correct but means the five are not five independent signals. ------------------------------------------------------------------------------- WHAT I COULD NOT CHECK ------------------------------------------------------------------------------- 1. THE --commit WRITE PATH WAS NEVER EXECUTED (review constraint). Phases 1-4 are REASONED from source only. No POST, PATCH or read-back behaviour is measured. In particular I did not observe how the apex responds to a duplicate ip-address POST, nor whether enforce_global_unique is set on this NetBox. 2. MAAS. The D-139 execution list step 6 reads "retire the ULA rows IN THE APEX AND IN MAAS". This tool is apex-only and its docstring does not claim otherwise. I did not check whether the MAAS half is covered by another tool or is still owed. FLAGGED as an open scope question for the operator, not as a defect of this tool. 3. Whether --dc vr1-dc1 is intended to be run in this window at all. DEF-1 is stated as a missing guard regardless of intent; if dc1 is out of scope the fix may simply be to restrict the flag. 4. C.NB._req with method="DELETE" was never exercised; I verified by reading that no caller passes it, not by running it. 5. Real-world contents of ~/vr1-office1-creds/vr1-netbox-sandbox.env -- sourced only, never read or printed, per the secrets rule. 6. Concurrency. If anything else writes the apex between the tool's initial read and its phase-3 patch, plan() is stale. Not evaluated. 7. The 2602:f3e2:f02:11:: provider-public VIP records' provenance -- I confirmed they EXIST and match the overlay, but did not trace which tool created them. ------------------------------------------------------------------------------- REPO INTEGRITY STATEMENT ------------------------------------------------------------------------------- No repo file was edited by this review. All mutation experiments were performed on copies under /home/jessea123/.claude/jobs/015c103e/tmp/mut/ , and the pre-mutation copies were diffed against the repo originals at the end of the run: IDENTICAL: netbox/d139-step6-vip-rehome.py IDENTICAL: netbox/d139-gua-carve.py IDENTICAL: tests/d139-step6-vip-rehome/run-tests.sh The harness was re-run against the real repo files afterwards: PASS=14 FAIL=0. bash scripts/repo-lint.sh -> 0 fail, 1 warn (the pre-existing L1 legacy-ASCII warn on docs/design-decisions.md), 657 files scanned. NOTE, stated because it would otherwise look like my doing: `git status --short` showed ONLY the two untracked targets at the start of this review, and later showed docs/CURRENT-STATE.md, docs/design-decisions.md and tests/HARNESS-MANIFEST as modified. I inspected those diffs: they are the status=active -> status=reserved correction and the HARNESS-MANIFEST entry for d139-step6-vip-rehome -- concurrent edits from the parent session, not from this review. No git add / commit / push was run. No cloud mutation of any kind was performed. All apex access was GET. ------------------------------------------------------------------------------- SUMMARY OF REQUIRED FIXES BEFORE --commit ------------------------------------------------------------------------------- MUST DEF-1 precondition: every target GUA /64 must exist as a prefix row for --dc MUST DEF-2 restore the SANDBOX_HOSTS apex-identity guard (reuse C.SANDBOX_HOSTS) MUST DEF-3 assert the expected 26 / 26 / 9 floor; partial must not exit 0 MUST DEF-4 harness must cover main()'s ordering and its two failure branches SHOULD DEF-5 make T13 assert positively so it cannot pass on a traceback SHOULD DEF-6 include status in the ALREADY / read-back predicate SHOULD DEF-7 extend T14's grep to the imported module MAY DEF-8..DEF-11 latent / cosmetic; record rather than fix if time is short The mapping logic, the never-delete posture, the status choice, the verbatim descriptions and the source-level phase ordering are all correct and were verified. This is a good tool missing its preconditions and missing the test for its own headline property.