d139-carve-review-20260801.txt
==============================
INDEPENDENT review-and-streamline pass over netbox/d139-gua-carve.py and
tests/d139-gua-carve/, which were BUILT BY ANOTHER AGENT this session and
committed at 64cd539. Reviewer did not write the code. Host: vcloud.
Date: 2026-08-01. Governing decision: D-139 ruling B (cited, not re-argued).

READ-ONLY with respect to the cloud and the apex: NO write of any kind was made
to NetBox. The only live traffic was the mandated re-run of the DRY RUN plus one
prefix-count read. Apex prefix count 139 before and 139 after -- the same number
the builder recorded, so nothing this pass did landed a row.

D-139 ruling A was found NOT ACHIEVABLE at current charm revisions (item 14 of the
changelog). Ruling B is orthogonal and STANDS, so this tool remains valid. Scope
was NOT widened into ruling A.

=======================================================================
VERDICT
=======================================================================
NOT "no change needed" -- but close to it. The tool is genuinely well built: the
refusals are real refusals, the dry-run default is structurally honest, the
read-back is load-bearing, and the prose is already at charter weight (5 comment
lines in 232). FOUR defects were found and FIXED; four more are REPORTED and
deliberately NOT fixed; the one criterion the builder declared as missed
(main() ~90 lines) was assessed and the miss is UPHELD as correct.

The single most valuable finding is D1: the most likely real-world failure of
this tool -- an unreachable apex -- exited with the WRONG code and a raw
traceback. That is the mandate's named class "a refusal that does not refuse".

=======================================================================
LINE COUNTS -- before / after
=======================================================================
  netbox/d139-gua-carve.py            232 -> 233   (+1)
  tests/d139-gua-carve/run-tests.sh   151 -> 166   (+15)
  tests/d139-gua-carve/stub-apex.py   100 -> 104   (+4)

The tool did not get shorter. That is the honest outcome: there was no prose to
cut. Its 232 lines are refusals, conflict detection, read-back and the write
path, exactly as the builder claimed. The +1 net is a 2-line correctness fix
minus a 1-line stale comment removal.

=======================================================================
DEFECTS FOUND AND FIXED (4)
=======================================================================

D1 -- REAL DEFECT (correctness + the three-outcome contract). An UNREACHABLE
     apex produced a raw Python traceback and exited 1.
     MEASURED BEFORE THE FIX, verbatim tail:
         urllib.error.URLError: <urlopen error [Errno 111] Connection refused>
         RC=1
     _req() caught only urllib.error.HTTPError. A URLError / socket timeout /
     DNS failure is not a RuntimeError, so it walked straight past main()'s
     `except RuntimeError -> die(...)` -- the handler that exists precisely to
     turn an unreadable apex into REFUSE(2) with the User-Agent hint.
     WHY IT MATTERS beyond tidiness: this tool's own header defines rc 1 as
     "write/read-back error". A DRY RUN that never reached the apex has written
     NOTHING, so rc 1 told the operator the opposite of the truth, in the single
     most likely failure mode (VPN down, NetBox restarting, wrong port).
     An unrecognised state defaulted to a WRITE verdict instead of REFUSING.
     FIX (2 lines, after the HTTPError clause -- order matters, HTTPError
     subclasses URLError subclasses OSError):
         except OSError as e:   # unreachable/timeout is UNEVALUABLE (rc 2), not a write error (rc 1)
             raise RuntimeError("%s %s -> %s" % (method, path, e))
     Now REFUSE(2), no traceback, and it names the failed read. New test T18.

D2 -- REAL DEFECT (record accuracy + charter). The comment above the D-139
     table asserted a GOVERNANCE STATE THAT WAS ALREADY FALSE when it was
     committed. It read:
         # --- D-139 ruling B (FLAGGED for operator reconsideration at 633da1c -- its stated
         # deciding reason was refuted by measurement; the table is D-139 as it stands today) --
     TIMELINE, measured from git (`git log --date=iso`):
         633da1c  00:11:12  ruling B's deciding reason REFUTED; ruling FLAGGED
         bbc0330  00:20:47  ruling B RECONSIDERED and CONFIRMED, unchanged in substance
         64cd539  00:55:25  the tool committed, still carrying the FLAGGED comment
     The reconsideration had closed 35 minutes before the tool landed. A reader
     grepping the tool would conclude ruling B is pending an operator decision.
     It is not: D-139's RULING NOTE 2026-08-01 confirms it, resting now on
     conformance-with-Willamette/VR0 and minimize-delta-to-Roosevelt.
     It is also the charter violation "a script CITES decisions; it never
     re-argues them" -- the comment annotated the decision's standing rather
     than citing it, which is exactly why it could rot.
     FIX: replaced with a bare citation, `# --- D-139 ruling B, table verbatim ---`.
     The reconsideration history was NOT written in its place; that would be
     re-arguing in the other direction. It lives in design-decisions.md.

D3 -- REAL DEFECT (a test that asserts EXISTENCE, and a guard never exercised).
     Old T16 was two greps over the TOOL'S OWN SOURCE TEXT:
         grep -q "instrument currency before negatives" "$S"
         grep -q 'if not scoped:' "$S"
     That asserts a string is present in a file. It does not prove the guard
     FIRES, that it exits 2, or that the operator ever SEES the message. No stub
     mode produced zero scoped prefixes, so the zero-scoped-read REFUSE -- a
     guard written against this repo's own recorded negative-misread class --
     had no behavioural coverage at all. It also breaks the charter rule
     "assert on CONTENT, never existence" in the harness that enforces it.
     FIX: new STUB_MODE=empty-scope models the REALISTIC cause (a filtered or
     failed read: rows come back, none scoped to the asked DC; the site still
     exists, so only the scoped count distinguishes it from a virgin DC). T16 is
     now three behavioural assertions: rc=2, the message reaches stdout, and
     -- the one that matters -- NO plan is printed, proving the tool refuses
     BEFORE emitting 14 confident CREATEs built on a read it does not trust.
     Both source-text greps DELETED.

D4 -- REAL DEFECT (vacuous negative assertions -- this repo's eight-time
     misread class, from MEMORY "instrument currency before negatives").
     T09 and T11 assert ABSENCE against a sed-extracted section:
         c="$(printf '%s' "$OUT" | sed -n '/^  CREATE:/,/^$/p')"   # T09
         r="$(printf '%s' "$OUT" | sed -n '/^  RETIRE/,$p')"       # T10/T11
     Nothing proved either extraction was non-empty. If a print-format change
     empties $c, all three T09 "would re-create existing $cidr" checks PASS --
     vacuously. Same for T11's three scope checks over $r, which are the tests
     standing between the report and sweeping the org ULA aggregate or a v4
     plane into a retirement list.
     FIX: one non-vacuity assertion per extraction. PROVEN LOAD-BEARING by
     mutation M8 below, which renames the CREATE section title and turns the
     new T09 guard red while every other T09 check stays green -- i.e. without
     the guard that mutation was INVISIBLE.

=======================================================================
REPORTED, NOT FIXED -- findings, logged not executed (CLAUDE.md rule 1)
=======================================================================

R1 -- MEASURED GAP in a control the mandate calls non-negotiable: the
     SANDBOX_HOSTS refusal gates the URL THE OPERATOR SUPPLIES, but urllib
     follows 3xx redirects by default and CARRIES THE Authorization HEADER
     ACROSS HOSTS. Not inferred -- measured with a two-server loopback probe:
         redirect target received Host   : localhost:33755
         redirect target received Authz  : YES -> Token SECRET-TOKEN-VALUE
     CPython's HTTPRedirectHandler.redirect_request strips only content-length
     and content-type (`CONTENT_HEADERS = ("content-length", "content-type")`),
     so the token survives a redirect to an arbitrary host.
     SEVERITY: LOW, and it is a gap not a hole -- exploiting it requires the
     APPROVED apex itself (10.10.1.10, on the trusted LAN) to issue a hostile
     redirect. It does not weaken the DOCFIX-195 two-instance protection, whose
     threat is operator confusion, not a malicious apex.
     NOT FIXED ON PURPOSE: a redirect handler is new, untested code on a control
     path, and adding it is scope this review was not given. If the operator
     wants it closed, the minimal form is an opener with a redirect handler that
     re-checks hostname against SANDBOX_HOSTS -- it needs its own test.

R2 -- The read-back `nb.get_all("/ipam/prefixes/")` in the --commit path is NOT
     wrapped in try/except. After the D1 fix it raises RuntimeError instead of
     URLError, so an apex that dies mid-commit still tracebacks and exits 1.
     The exit code is now CORRECT (writes did happen; rc 1 is "write/read-back
     error" and this is literally a read-back error) but the output is a
     traceback where a sentence is owed. NOT FIXED: it is on the write path,
     which cannot be exercised live under this mandate, and contriving a
     mid-run stub kill to test it buys less than it risks.

     DECLARED SIDE EFFECT OF D1 ON THE WRITE PATH -- stated because "a
     streamline that changes behaviour is a bug" is the standard this pass was
     held to, and this IS a behaviour change, deliberate and out of scope to
     undo. _req() is shared by the --commit POST loop, whose handler is
     `except RuntimeError`. BEFORE the fix a connection error during a POST
     raised URLError, escaped the loop, and ABORTED the run on the first
     failure with a traceback. AFTER the fix it is a RuntimeError, so it is
     caught, counted into errs, AND THE LOOP CONTINUES TO THE NEXT POST.
     Consequences, both real: (i) against a black-holed apex all 11 POSTs are
     attempted at timeout=45 each, roughly 8 minutes, plus another 45s in the
     unwrapped read-back; (ii) `RESULT: errors=N` with rc 1 is now reachable in
     a state where NO write was ever transmitted, which widens what that tally
     means. NOT FIXED: making the loop abort on a transport failure is new,
     untested code on the write path -- the same reasoning as R2 above.
     THIS IS UNTESTED. The evidence in VERIFICATION below does not cover it.

R3 -- `errs += len(gone)` double-counts a POST that both failed AND is absent
     on read-back. Affects only the `RESULT: errors=N` tally; the exit code
     (1 if errs else 0) is unaffected either way. Write-path edit for a
     cosmetic gain -- deliberately not taken.

R4 -- `net.subnet_of(ula)` is TRUE for the /48 itself, so an org ULA aggregate
     that were ever DC-scoped would be swept into the RETIRE report. MEASURED
     NOT TO OCCUR: this run's RETIRE-REPORT is 18 rows, all /60 and /64, no /48
     -- the aggregate is unscoped live, exactly as the stub fixture models it.
     Observation, not a defect: the tool never deletes, so the worst case is
     over-reporting, and the harness pins the correct behaviour (T11).

R5 -- The committed capture docs/audit/d139-carve-dryrun-20260801.txt carries a
     STANDING NOTE saying the --commit push "is gated on that reconsideration".
     That was TRUE when written (00:18) and became STALE at 00:20 when bbc0330
     confirmed the ruling. NOT EDITED -- it is a verbatim GA-R1 record and was
     accurate at capture time. Recorded here instead. The live gate today is
     ordinary operator approval, not a pending reconsideration.

=======================================================================
main() DECOMPOSITION -- assessed, and the builder's declared MISS is UPHELD
=======================================================================
The builder declared: "One soft criterion missed and declared: main() is ~90
lines, so one screen per function does not hold there." main() is lines
140-229, i.e. 90 lines. The criterion IS missed. It should stay missed.

The candidate extractions were connect() (env + host REFUSE -> NB) and
commit_writes() (POST loop + read-back), which together would bring main() to
roughly 55 lines.

DECLINED, and the discriminator is not line count -- it is whether a reader can
verify a SAFETY property in fewer places or more:
  - Today `if not a.commit: return 0` and the entire write block are ADJACENT.
    One glance confirms "nothing writes unless --commit". Extracting
    commit_writes() splits that proof across two locations, for a script whose
    whole claim is that the write path is opted into.
  - connect() separates the SANDBOX_HOSTS refusal from its point of use. The
    refusal's value is that it visibly precedes the construction of NB(url,
    token) -- so the token is never even handed to a client for a bad host.
    Moving it into a helper makes that ordering a thing you must go and check.
  - main() is a LINEAR pipeline with no branching complexity: parse, refuse,
    read, resolve, plan, print, optionally write. Helpers each called exactly
    once, in order, are scattering, not decomposition.
For a 232-line safety-critical script that reads like a runbook, linear beats
decomposed. The miss is the right trade and should be recorded as accepted
rather than silently carried.

=======================================================================
CHARTER CRITERIA -- before / after
=======================================================================
  Cites decisions, never re-argues     PARTIAL -> MET   (D2: the FLAGGED
                                                         annotation removed)
  Header 25 lines max                  MET (13) -> MET (13)
  No comment restates the code         MET -> MET
  No comment block over 5 lines        MET (max 2) -> MET (max 2)
  No examples / alternatives / vendors MET -> MET
  ~one screen per function             MISSED (main 90) -> MISSED, UPHELD as
                                       the correct trade (see above)
  Assert on CONTENT, never existence   VIOLATED in the HARNESS (D3: T16 grepped
                                       the tool's source) -> MET
  PASS / FAIL / REFUSE, no default-ok  VIOLATED (D1: unreachable apex exited 1
                                       with a traceback) -> MET
  Dry-run is the DEFAULT               MET -> MET (unchanged, re-proven T06)
  Host hard-refusal {localhost,
    127.0.0.1, 10.10.1.10}             MET -> MET (untouched; see R1)
  Credentials sourced, never printed   MET -> MET
  User-Agent curl/8.5.0 pinned         MET -> MET (untouched)

=======================================================================
THINGS THE BUILDER GOT RIGHT THAT ARE WORTH RECORDING
=======================================================================
- get_all() RE-ANCHORS pagination to the approved host:
      nxt = n.split("/api", 1)[1] if n else None
  The apex's `next` URL is reduced to a PATH and re-joined to self.url, so a
  crafted next cannot walk the tool off the approved host. That is a real
  safety property and the builder did not claim it. (Note it does not help R1,
  which is a transport-layer redirect, not an application-layer next link.)
- The sub_at() /60-alignment die() is NOT a dead assertion. Mutation M1 (storage
  hextet 0x40 -> 0x41) makes it FIRE and REFUSE. An assertion that can be shown
  to fail under a plausible edit is doing work.
- build_plan() runs BEFORE build_retire(), and both run BEFORE any printing, so
  the zero-scoped REFUSE lands before a single plan line is emitted. Proven by
  the new T16, not assumed.
- CONFLICT is treated as an UNRECOGNISED STATE that REFUSES (rc 2), not as a
  no-op "already exists". That is the hard call and it was made correctly.

=======================================================================
VERIFICATION -- real numbers
=======================================================================
HARNESS      65/65 PASS (baseline, before any edit)
             71/71 PASS (after)
             Delta +6 = -2 (both T16 source greps DELETED)
                        +3 (T16 rebuilt behaviourally, STUB_MODE=empty-scope)
                        +3 (T18 unreachable apex -> REFUSE)
                        +2 (T09/T10 non-vacuity guards)

MUTATION PASS. The tool was edited, so the builder's 5 results are no longer
evidence for the changed file. ALL FIVE WERE RE-RUN against the edited tool, and
three new mutations cover the new paths. Restore was by copy from a pristine
snapshot (NOT git checkout -- this clone is shared with concurrent sessions);
sha256 verified IDENTICAL after every single one.
  pristine/final sha256 38484b52a5e660b3a70dc0376fc06041c97ed0e2153d4519f511af660d9bd3d3

  M1  storage hextet 0x40 -> 0x41           RED T07, T08(x11), T05  48/71 FAIL
      (the /60-alignment guard REFUSEs, which is why T05's rc=0 also goes red)
  M2  UA pin line DELETED                   RED T15 + cascade       53/71 FAIL
  M3  SANDBOX_HOSTS refusal neutered        RED T04 ONLY             1/71 FAIL
  M4  retire predicate ULA/48 -> GUA/36     RED T10(x10), T07, T12  12/71 FAIL
  M5  read-back gone = []                   RED T17(x3)              3/71 FAIL
  M6  NEW: OSError catch removed            RED T18(x3)              3/71 FAIL
      (i.e. M6 restores the pre-review behaviour and the new test catches it)
  M7  NEW: zero-scoped-read guard neutered  RED T16(x2)              2/71 FAIL
  M8  NEW: CREATE section title renamed     RED T09 non-vacuity      12/71 FAIL
      guard + T12; PROVES D4 -- without the guard this mutation silently
      passed T09's three absence checks.
  All 5 original mutations STILL STAND on the edited tool. None was carried
  over on the builder's word.

REPO-LINT   before: 0 fail, 1 warn, 645 files  (the warn is L1
            docs/design-decisions.md 239 non-ASCII bytes -- the pre-existing
            legacy D-001..018 carve-out, NOT caused by this pass)
            after, code + harness edits ONLY: 0 fail, 1 warn, 645 files
            -- the EXPECTED result, unchanged.

            after, WITH THIS CAPTURE FILE PRESENT: 1 fail, 1 warn.
              [FAIL] L10 pending change set modifies state-bearing surface(s)
              [docs/audit/: docs/audit/d139-carve-review-20260801.txt] without
              touching docs/CURRENT-STATE.md -- GA-R1 rule 8 (C1)
            THIS IS NOT A REGRESSION AND NOT A DEFECT IN THE TOOL. It is the
            mandate's own two constraints colliding: this review was REQUIRED to
            record its findings in docs/audit/ and FORBIDDEN to touch
            docs/CURRENT-STATE.md. L10 is working exactly as designed.
            CAUSATION MEASURED, not inferred: the capture was moved aside, lint
            re-run (0 fail, 1 warn, 645 files), and the capture moved back with
            sha256 verified identical. The code and harness edits alone are
            lint-clean; the FAIL is caused solely by this file's existence in an
            uncommitted change set.
            RESOLUTION IS THE OPERATOR'S: add the CURRENT-STATE.md paragraph in
            the SAME commit that lands this capture, which is what rule 8 asks
            for and what a reviewer forbidden to touch that file cannot do.

GAUNTLET    ALL GREEN (95 harnesses). No harness added or removed, so the
            R15(2) HARNESS-MANIFEST is undisturbed.

LIVE DRY RUN, both DCs, --commit NEVER passed:
    vr1-dc0  apex prefixes scoped: 21   CREATE 11 | EXISTS 3 | RETIRE-REPORT 9
             dependent: 26 ip-address(es), 0 ip-range(s)   rc=0
    vr1-dc1  apex prefixes scoped: 21   CREATE 11 | EXISTS 3 | RETIRE-REPORT 9
             dependent: 26 ip-address(es), 0 ip-range(s)   rc=0
    TOTALS   CREATE 22 | EXISTS 6 | RETIRE-REPORT 18, 52 dependent ip-addresses
    UNCHANGED from the builder's capture -- and asserted on CONTENT, not on the
    counts alone: the plan body was DIFFED against the committed capture
    (lines 25-97) and is byte-for-byte IDENTICAL.

    SCOPE OF THAT CLAIM, stated precisely rather than left to be assumed: the
    byte-identical dry run proves the READ AND PLAN PATH is unchanged. It
    proves NOTHING about the write path, because a dry run executes zero POSTs
    by construction. The write path's FAILURE semantics DID change as a
    declared side effect of D1 (see R2), and that change is UNTESTED. The
    happy write path is unchanged. So: "behaviour preserved" is proven for
    everything --commit does not touch, and asserted-with-a-declared-exception
    for what it does.
    NO WRITE: apex prefix count 139, the same value recorded before and after
    the builder's run.

=======================================================================
TRANSCRIPT-ONLY NOTES -- recorded so they do not die with the session
=======================================================================
- New T16's `has "CREATE" && bad || ok` is itself a NEGATIVE assertion, and it
  is non-vacuous ONLY because the preceding `[ "$RC" -eq 2 ] && has "instrument
  currency..."` already forces $OUT non-empty. That is the same anchoring
  argument that makes T12's negative safe. A later editor who REORDERS those
  assertions would quietly re-introduce D4 inside the very test written to fix
  D4. Keep the rc/message check FIRST.
- The R1 redirect probe was a throwaway two-server loopback script in /tmp with
  a fake token; no repo file and no real credential was involved, and no traffic
  left the host.
- The no-write verification used `curl -H "Authorization: Token $NETBOX_TOKEN"`,
  which puts the token in the process table transiently. Nothing was printed and
  nothing reached the transcript, but that is the SAME exposure class this
  tool's own header warns about for argv. A `--netrc` or header-from-stdin form
  would be strictly better next time.
- The `python3 -m scripts` / caveman compressor was NOT invoked against anything
  (CLAUDE.md tooling guardrail); every edit here is a targeted hand edit.

=======================================================================
NOT DONE, ON PURPOSE
=======================================================================
- No write to NetBox, at all. --commit was never passed.
- No git add / commit / push / checkout (concurrent sessions share this clone).
  The working tree is left MODIFIED and uncommitted for the operator.
- docs/CURRENT-STATE.md, docs/design-decisions.md, scripts/dc-snap-proxy.sh and
  tests/dc-snap-proxy/ were not touched.
- The committed dry-run capture was not rewritten (see R5).
- No changelog entry was written: the delivery discipline requires one for a
  script change, but the changelog is a shared surface mid-session and this pass
  was scoped to review. OWED: a changelog item covering D1-D4 with the revert
  `git checkout netbox/d139-gua-carve.py tests/d139-gua-carve/`.
