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: 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/`.