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