Newer
Older
openstack-caracal-ipv4 / docs / script-quality-findings-20260707.md

Script-quality findings register -- read-only sweep, 2026-07-07

What this is. The deferred-findings register from the 2026-07-07 read-only script-quality sweep, recorded so the items survive session compaction. Nothing in this file has been executed; every fix listed here is a DOCFIX candidate to be delivered later under the standard change-delivery loop (harness green, repo-lint clean, changelog entry with revert). No identifier numbers are assigned in this register -- the batch that fixes an item assigns its number via the next-free rule at delivery time.

Sweep coverage. 110 files reviewed (scripts/, tests/, .claude/hooks/). Tools available on the review host: bash and python3 only; shellcheck and pyflakes were absent, so findings come from manual read plus targeted fixture probes, not static analysis.

Batch note. The sweep's E/R-class items approved for immediate fix were implemented in the fix batch of this date and are NOT listed here, except the two previously-known items marked FIXED-THIS-BATCH below.

Deferred findings (R-class; fixes are DOCFIX candidates)

R5 -- lib-validate.sh: vr_json tempfile leak

vr_json (scripts/lib-validate.sh, ~line 52) creates _VR_ERR="$(mktemp)" on every call and never removes it. vr_err_tail legitimately needs the file after return, but nothing cleans it up afterwards: one leaked tempfile per vr_json call, per check, per validation run. Fix shape: a single per-process stderr file reused across calls, or an EXIT trap in the library's sourcing contract.

R6 -- | head -1 SIGPIPE / extract-then-check class in phase-05/06

Live-pipe openstack ... | head -1 captures that use the fragment without whole-output validation:

  • scripts/phase-05-amphora-pipeline.sh:62 (image list --tag ... | head -1)
  • scripts/phase-05-amphora-pipeline.sh:66 (image list --name ... | head -1)
  • scripts/phase-06-mgmt-vm.sh:106 (port list --server ... | head -1)
  • scripts/phase-06-mgmt-vm.sh:108 (floating ip list --port ... | head -1) Same class the house style forbids (SIGPIPE race on the producer; a partial failure yields a plausible-looking fragment). Fix shape: capture whole output, validate shape (uuid regex), then select the first row.

R7 -- phase-07-conductor-graft.sh: remote tempdir leak

The helm-install payload run on the conductor (~line 153) does D=$(mktemp -d); cd "$D" and never removes the directory: one leaked tempdir (plus tarball) on the remote unit per graft run. Fix shape: trap cleanup inside the remote payload.

R9 -- cloud-assert.sh --capture: --long + silent empty images.json

Line ~184: openstack image list --long -f json </dev/null > "$DIR/images.json" 2>/dev/null || true. Two defects: (a) the deprecated --long flag only adds stderr noise (and 2>/dev/null then masks REAL errors, against the stderr-separation rule); (b) a failed list silently commits an empty/missing images.json into the captured BOM. Fix shape: drop --long, stderr to a tempfile surfaced on failure, fail the capture loudly if the JSON is empty or unparseable.

R11 -- harness tempfile leaks

Harnesses that mktemp without a trap cleanup (leak per run):

  • tests/carve-host-interfaces/run-tests.sh
  • tests/lib-validate/run-tests.sh (also amplifies R5: every vr_json case leaks)
  • tests/phase-00-maas-standup/run-tests.sh Fix shape: the standard W=$(mktemp -d); trap 'rm -rf "$W"' EXIT pattern the other 34 harnesses already use.

Previously-known items -- FIXED-THIS-BATCH

  • tenant-offboard.sh Phase B counter (FIXED-THIS-BATCH). Logged in the changelog (2026-07-06 addendum 28) as FOUND-NOT-FIXED: the residual-trust sweep incremented SWEEP_FAIL inside a printf | while pipeline subshell, so failed trust deletes could never raise exit 22. Fixed in this date's batch (here-string loop; regression case in tests/tenant-offboard).
  • tenant-offboard.sh hardcoded auth URL (FIXED-THIS-BATCH). The known hardcoded-OS_AUTH_URL class (house rule 3): tenant_env now reads auth_url= from the cred file with the literal demoted to a KEYSTONE_VIP-overridable fallback. NOTE: the tenant-acceptance.sh foil_env sibling of this class (addendum 28 FOUND-NOT-FIXED list) remains OPEN and is NOT covered by this batch.

Consolidation proposals (S-class; S1-S8)

Transferred verbatim from the sweep report at integration (2026-07-07); effort estimates are the sweep's (S/M/L). Operator has NOT ruled on S1-S5/ S7/S8 -- proposals only.

  • S1 (M) The unset-OS_/source-env dance is hand-rolled ~15x across tenant-onboard (8 subshells), tenant-offboard (2), tenant-acceptance (3), d063-apply (1), tenant-assert (2) -- while scripts/lib-validate.sh already ships tested vr_scrub_os / vr_admin_env / vr_tenant_env. Proposal: source lib-validate.sh from the tenant- family + d063-apply; add one vr_appcred_env <file> for the app-cred variant. (The offboard tenant_env auth-url bug was fixed narrowly in this batch; this proposal remains the wider consolidation.)
  • S2 (S) run() capture-then-test helper duplicated with eval-counter variants in tenant-offboard.sh and d063-apply.sh vs the rc-returning lib-validate run(). Consolidate; count failures at call sites.
  • S3 (S) is_id() defined in 4 scripts (tenant-onboard, tenant-offboard, d063-apply, tenant-assert); lib-validate already has vr_is_hex32. Ride along with S1.
  • S4 (M) The trust-list stdout-only capture + inline-python filter block is near-identical 3x (tenant-offboard x2, tenant-acceptance x1) and is inline python-in-bash beyond one-liner scale (house rule: .py files). One fixture-tested scripts/trust_filter.py would serve all three.
  • S5 (S) Kube-image UUID resolution block (json capture + jq kube filter
    • tempfile + 36-hex validation) appears 3x inside tenant-onboard.sh. One local function.
  • S6 (no action) cap()/oerr() triplicated across clientdocs/scripts/ {smoke-test,acceptance-run,ci-cleanup-sweep} -- CORRECT as-is: handover artifacts must be standalone off-jumphost. Worth a header comment noting the duplication is deliberate.
  • S7 (S) Dead code: checks/d011-04 CF assigned never used; checks/d011-03 vr_is_ipv4-or-VHOST first test subsumed by second. (The other two S7 items -- offboard no-op is_id, validate.sh lost line-rewrite -- were FIXED in this batch.)
  • S8 (S) Move the KEYSTONE_VIP default and the D-003 FIP pool bounds into lib-net.sh as tagged constants (the literal is duplicated in 4 files: tenant-onboard, phase-03-admin-openrc, phase-06-kubeconfig-gate, phase-06-k8s-bootstrap; FIP pool literals duplicated across the two phase-04-network scripts; provider-bundle-check.py re-states plane CIDRs as a second source of truth -- python cannot source bash, needs a generated or parsed form).

Found during the fix batch (this date; logged, NOT fixed -- out of scope)

  • tenant-offboard.sh Phase A cluster captures merge stderr. The tenant-scope coe cluster list captures (inventory ~line 160; delete loop and wait loop in Phase A) still use 2>&1: a stderr warning line would word-split into coe cluster delete argv and keep the wait loop from ever seeing empty (budget exit 21). Same class as the Phase C fix; was not in the approved batch scope. The offboard harness deliberately excludes coe commands from its stderr-noise scenario until this is fixed.
  • Merged-stderr domain show/token issue captures (fail-closed class). tenant-offboard.sh (~line 133, sweep-mode preamble) and tenant-onboard.sh stage subshells validate $(... 2>&1) captures by shape/equality. Failure direction is CLOSED (safe), but a benign stderr warning on a noisy CLI would abort with a false precondition/auth failure. Cosmetic-risk cleanup candidate, same stdout-only pattern.
  • tenant-offboard.sh Phase E0 app-cred inventory merges stderr. Line ~267 tests rc (failures route correctly), but a success-with-warning would word-split the warning into the display-only cascade-inventory lines. Display pollution only; no delete argv exposure.

ASCII + LF.