Replace the seven duplicated 8-byte-hex random ID generators (interna...

[colonelpanik]

Goal

Replace the seven duplicated 8-byte-hex random ID generators (internal/failover/coordinator.go:1679 newID, internal/grpcapi/host.go:934 newID, internal/grpcapi/users.go:436 generateID, internal/scheduler/rebalancer.go:709 newID, internal/grpcapi/notifications.go:91 newNotifyID, internal/health/owner_assert.go:335 ownerAssertID, internal/ui/handle_security_groups.go:173 newUIID) with one shared helper, picking a single explicit crypto/rand.Read error policy instead of today's mix of silent-discard (six sites) and propagate-error (newUIID, whose own comment says it 'matches the lv CLI's newID scheme').

Plan

Plan: one shared 8-byte-hex random ID helper

Replace the seven copy-pasted 8-byte-hex ID generators named in the task — plus
an eighth the task did not name, cmd/litevirt/sg.go:244, for the reasons in
§5 — with a single leaf-package helper, and settle the error policy that today is
split six-to-one.

Nothing about the generated ID changes: 8 bytes from crypto/rand, hex-encoded,
16 lowercase hex characters. Every one of these IDs is a primary key in a
replicated Corrosion table, so the shape is not ours to change (see
Reviewer notes).

---

1. The decision: the helper returns a bare string, never an error

Today six sites discard the crypto/rand.Read error and one
(internal/ui/handle_security_groups.go:173 newUIID) propagates it. The plan
picks discard — structurally, by not having an error to return.

This is not a coin flip. Three independent reasons:

a. The Go 1.26 stdlib contract makes the error leg unreachable. go.mod
declares go 1.26.0, and in this toolchain crypto/rand.Read is documented:

Read fills b with cryptographically secure random bytes. It never returns an
error, and always fills b entirely.

Read calls io.ReadFull on Reader and crashes the program irrecoverably if an
error is returned.

($(go env GOROOT)/src/crypto/rand/rand.go:41-46.) The stdlib has already taken
over failure handling, and it does so by killing the process. A caller-visible
error return cannot fire. Propagating it isn't defence in depth; it is dead
code that reads like a live path.

b. The repo has already voted, twice. internal/ui/handle_firewall.go:201
adds a whole wrapper for the sole purpose of throwing the error away, and its
comment says so:

// mustUIID returns a random hex id. newUIID only errors if crypto/rand fails,
// which doesn't happen on a real host; the empty-string fallback is never hit
// in practice.
func mustUIID() string { id, _ := newUIID(); return id }

Four of the six newUIID call sites already use id, _ :=. The error return has
produced one wrapper and four ignored values, and zero handled failures.

c. The propagate policy is the more dangerous of the two here. newUIID
returns ("", err). mustUIID drops the error and returns "". That empty
string then goes in as a primary key —
corrosion.InsertFirewallRule(... ID: mustUIID() ...)
(internal/ui/handle_firewall.go:74,106,145). In an LWW-merged CRDT store every
row that took that path collides on the same empty-string PK and silently merges
cluster-wide. The no-error helper cannot produce that state, because there is no
failure path to fall out of. Removing the error return closes a latent
fail-open bug rather than opening one.

The honest counter-argument, for the record: dropping an error normally loses
information. It doesn't here — the stdlib made the promise and owns the failure —
and this is the same reasoning the stdlib itself used when it changed Read's
contract. math/rand is nowhere in this change, so there is no
silently-weaker-randomness risk.

Implementation shape. _, _ = rand.Read(b) with a comment citing the
contract. Explicitly not:

• a panic on error — unreachable, and it invites a reviewer to ask for a test
  that cannot be written;
• rand.Text() (Go 1.24+) — base32, 26 chars, would change the persisted format;
• github.com/google/uuid — excluded by the task; also longer and it would
  change every persisted ID.

The //nolint:errcheck markers on four of the current sites can go: _, _ = is
an explicit discard that errcheck accepts. (This repo has no .golangci.yml
and CI runs only go vet ./..., so those markers are for contributors' local
linters, not a gate — but the shared helper should be lint-clean anyway.)

---

2. The new package: internal/randid

A stdlib-only leaf, modelled directly on internal/safename, which exists for
exactly this reason and says so in its own doc comment: "It is zero-dependency
(stdlib only) so any package can import it without risking an import cycle […]
This generalizes the per-package withinDir/safeJoin helpers." Same precedent,
same justification. internal/failover, internal/grpcapi, internal/scheduler,
internal/health, internal/ui, and cmd/litevirt can all import it; it
imports nothing from the repo, so no cycle is reachable even in principle.

Name: package randid, function New(). randid.New() reads cleanly at ~50
call sites; randid.NewID() stutters.

internal/randid/randid.go (new)

// Package randid generates the short random identifiers litevirt uses as
// primary keys for replicated rows — audit entries, firewall and
// security-group rules, notification targets and routes, reservations,
// scheduler proposals, relocation tokens.
//
// It is zero-dependency (stdlib only) so any package can import it without
// risking an import cycle. It replaces eight byte-identical per-package
// generators (failover.newID, grpcapi.newID, grpcapi.generateID,
// grpcapi.newNotifyID, scheduler.newID, health.ownerAssertID, ui.newUIID,
// and main.newID in cmd/litevirt/sg.go, which originated the scheme) that had
// drifted into two different crypto/rand error policies.
package randid

import (
	"crypto/rand"
	"encoding/hex"
)

// Bytes is the number of random bytes behind one ID. Do not change it: every
// ID this package has ever produced is a primary key in a replicated
// Corrosion table, and shrinking it raises the collision probability for rows
// that already exist. 8 bytes = 64 bits of entropy = 16 hex characters.
const Bytes = 8

// New returns Bytes cryptographically random bytes, hex-encoded — 16 lowercase
// hex characters.
//
// It returns no error, and that is deliberate. As of Go 1.24 crypto/rand.Read
// "never returns an error, and always fills b entirely"; it crashes the program
// irrecoverably if the system entropy source fails. There is therefore no
// caller-visible failure to report, and an error return would be unreachable
// code. Do not add one back: the previous error-returning variant only ever
// produced an empty string that callers inserted as a primary key.
func New() string {
	b := make([]byte, Bytes)
	_, _ = rand.Read(b) // cannot fail; see the doc comment
	return hex.EncodeToString(b)
}

Not folded in, per the task and because each differs in size and purpose:
internal/daemon/daemon.go:1731 generatePassword (16 bytes, admin password),
internal/grpcapi/users.go:280 newSessionID (32 bytes, session token),
internal/pbsstore/chunkstore.go:477 randomChunkSuffix (4 bytes, test-only),
and the inline 32-byte API-token generation at internal/grpcapi/users.go:386.

---

3. Files to change

50 call-site rewrites — 48 in 20 production files, 2 in test files.
grep finds 57 references to the seven generators under internal/; the other
7 are not rewritten but disappear: 1 is the newUIID() call inside the
mustUIID wrapper that this change deletes, and 6 live inside the four
superseded tests (§3, test-files table). Counts verified in
Appendix A.

Mechanical, but the import bookkeeping differs per file, so it is spelled out.

Column meanings. Sites = calls to rewrite. Drop imports = whether the
generator was the only user of crypto/rand and encoding/hex in that file (if
yes, both imports must go or the build breaks on unused imports — verified per
file).

Definitions to delete

File	Delete	Sites in file	Drop crypto/rand + encoding/hex
internal/failover/coordinator.go	newID (1678–1683)	13	yes
internal/grpcapi/host.go	newID (933–938)	2	yes
internal/grpcapi/users.go	generateID (436–440)	1	no — newSessionID:280 and the 32-byte token:386 still use both
internal/grpcapi/notifications.go	newNotifyID (91–95)	2	yes
internal/scheduler/rebalancer.go	newID (708–713)	1	yes
internal/health/owner_assert.go	ownerAssertID (335–339)	1	yes
internal/ui/handle_security_groups.go	newUIID (172–179)	2	yes
internal/ui/handle_firewall.go	mustUIID (201–206)	3	n/a (never imported them)

mustUIID disappears with the error return that motivated it — its three
callers call randid.New() directly.

Call sites only (add the import, rewrite the calls)

File	Lines
internal/failover/coordinator.go	882, 1014, 1040, 1108, 1177, 1199, 1249, 1270, 1363, 1377, 1527, 1529, 1622
internal/grpcapi/host.go	612, 674
internal/grpcapi/stacks.go	194, 205, 216, 226
internal/grpcapi/firewall_rules.go	55, 104, 151
internal/grpcapi/reservation_admission.go	357, 444, 638
internal/grpcapi/lb.go	606, 1956, 2107
internal/grpcapi/migrate_container.go	392, 394
internal/grpcapi/users.go	396
internal/grpcapi/notifications.go	116, 186
internal/grpcapi/streamevents.go	223
internal/grpcapi/vm_events.go	34
internal/grpcapi/promote.go	122
internal/grpcapi/project_authority_admission.go	187
internal/grpcapi/registry_creds.go	55
internal/scheduler/rebalancer.go	347
internal/health/owner_assert.go	325
internal/health/container_owner_assert.go	295
internal/ui/handle_security_groups.go	75, 129
internal/ui/handle_firewall.go	74, 106, 145
internal/ui/handle_notifications.go	65, 156

Control-flow changes (the only non-mechanical edits)

Two UI handlers currently branch on the unreachable error. Both branches are
deleted, not kept:

• internal/ui/handle_security_groups.go:75-80 (handleCreateSG)
• internal/ui/handle_security_groups.go:129-134 (handleAddSGRule)

Each is:

id, err := newUIID()
if err != nil {
    sendToast(w, "id generation failed", "error")
    w.WriteHeader(http.StatusInternalServerError)
    return
}

becoming id := randid.New(). No test asserts the "id generation failed"
toast (grepped: the string appears only in the two handlers), so no test
changes fall out of this. The remaining err-shadowing in those functions is
the corrosion.Insert* error, which stays checked — worth a careful read
during review that removing the earlier err := does not turn a later
if err := ... into a compile error. It does not: both later uses are their own
if err := corrosion.X(...); err != nil statements.

Test files

File	Change
internal/grpcapi/host_test.go	delete TestNewID_UniqueAndLength (347–356) — superseded by the randid tests
internal/grpcapi/users_test.go	delete TestGenerateID (190–199) — superseded
internal/grpcapi/grpcapi_coverage_test.go	delete TestGenerateID_Unique (3673–3682) and TestNewID_Unique (3822–3831), and their now-empty ─── generateID … / ─── newID ─── section banners
internal/grpcapi/resize_admission_test.go	24, 32 — newID() → randid.New() (used as an op-ID argument)

Four scattered uniqueness/length tests collapse into one authoritative set in
internal/randid. Coverage of the generator is strictly better after this
(see §4); coverage of grpcapi drops by four trivial tests, and CI has no
coverage gate (.github/workflows/ci.yml runs build, vet, test, guards only).

Two comments name the old function and should be updated so they don't send a
future reader looking for a symbol that no longer exists:
internal/grpcapi/reservation_admission_test.go:102 and
internal/grpcapi/overcommit_reservation_test.go:21 (both say "ids come from
newID()").

---

4. Tests

4a. internal/randid/randid_test.go (new)

The format tests are the ones that matter — they are what stops a future edit
from changing a persisted ID shape.

Test	Asserts	Kills
TestNewDecodesToExactlyEightBytes	hex.DecodeString(New()) succeeds and yields len == 8	any size change; any non-hex encoding
TestNewLengthAndAlphabet	regexp ^[0-9a-f]{16}$ over 100 draws	uppercase hex, base32/base64, padding, a stray prefix
TestNewUnique	10,000 draws, zero collisions in a map[string]bool	a stubbed/zeroed generator (all-zero b collapses every ID to 0000000000000000), and a math/rand swap with a fixed seed
TestBytesConstantIsEight	Bytes == 8	a "harmless" constant tweak, with the doc comment explaining why it's pinned

TestNewUnique is the entropy test in practical terms: the realistic failure
mode is not a biased generator, it is a generator that stopped generating.

4b. internal/randid/drift_guard_test.go (new) — the part that makes it stick

A dedup that isn't guarded drifts back. This repo already institutionalises
source-scanning guards (scripts/ci/writecheck, scripts/ci/stmtshapecheck,
cmd/litevirt/docs_triangulation_test.go), so the pattern is established.

TestNoLocalEightByteHexIDGenerators walks internal/, cmd/, and tests/
from the module root — reusing the repoRoot(t) walk-up-to-go.mod idiom at
cmd/litevirt/docs_triangulation_test.go:585 — parses each production .go
file with go/parser, and fails on any function body that contains all three
of make([]byte, 8), a rand.Read call, and hex.EncodeToString. Files under
internal/randid are skipped.

Deliberately narrow, so it needs no allowlist: it keys on the literal size
8, and the three generators the task excludes are 16, 32, and 4 bytes. The only
other make([]byte, 8) in the repo is internal/qcow2/header.go:232
(writeEndOfExtensions), and internal/qcow2 imports neither crypto/rand nor
encoding/hex, so it cannot match. No innocent code trips it.

The guard scans cmd/, so it forces the eighth generator to go.
cmd/litevirt/sg.go:244 newID has all three predicates in one body —
make([]byte, 8), rand.Read(b), hex.EncodeToString(b). That is a true
positive: it is the duplicated generator, not innocent code. But it means the
guard cannot be green while that function exists, so the CLI migration is a
required step, not the optional one it was in an earlier draft of this plan
(§6 step 2, §5). The guard and the CLI migration are one decision, not two.

The alternative — scoping the guard to internal/ and leaving the CLI alone —
is spelled out as the fallback in §5 under "If the scope widening is
vetoed". It is the worse option: it puts the guard's blind spot exactly where
a known duplicate lives, which is the one place a guard has to see.

It lives as a normal go test in the new package rather than a new
scripts/ci/ binary, so go test ./... and CI pick it up with no Makefile or
workflow edit.

If a reviewer judges this scope creep, it can be dropped — but not
independently: dropping the guard is what makes the CLI migration optional
again. I recommend keeping both: the task is a drift fix, and the guard is the
only part that prevents the ninth and tenth copy.

4c. Mutation verification (required by CLAUDE.md)

"A passing test proves nothing until you have seen it fail."

Each mutation is applied, the named test confirmed red, then reverted:

Mutation	Must fail
Bytes = 4	TestNewDecodesToExactlyEightBytes, TestNewLengthAndAlphabet, TestBytesConstantIsEight
delete the rand.Read line (leave b zeroed)	TestNewUnique
hex.EncodeToString → base64.StdEncoding.EncodeToString	TestNewLengthAndAlphabet
paste a local newID (the old body) back into internal/health/owner_assert.go	TestNoLocalEightByteHexIDGenerators
point the guard's scan at a nonexistent subdirectory	the guard must fail loudly, not silently pass on zero files — mirrors the "found no litevirt_* identifiers … scan path wrong?" self-check at docs_triangulation_test.go:84

That last one is the vacuous-test trap CLAUDE.md warns about: a source-walking
guard that finds no files passes, and reads like a pass. The guard asserts a
non-zero file count for the same reason the docs guard does.

4d. Integration proof

The change is a refactor with no behavioural delta, so the existing suites are
the proof that call sites still work end to end. The multi-node paths that
consume these IDs — failover audit entries and relocation tokens, scheduler
proposals — are covered by tests/fleet/ (failover_test.go,
failover_invariants_test.go, failover_dispute_test.go,
audit_evidence_test.go), which CI runs. No new fleet test is warranted: no
failure mode introduced here is multi-node, because the ID bytes are unchanged.

4e. Full verification sequence

go build ./... && go vet ./...          # unused-import fallout surfaces here
go test ./...
make ci-guards

Then confirm the diff is format-neutral:

# every touched site produces 16 hex chars and nothing else
go test ./internal/randid/ -run TestNew -v
# no 8-byte generator survives outside internal/randid
go test ./internal/randid/ -run TestNoLocalEightByteHexIDGenerators
# no stragglers
grep -rn "make(\[\]byte, 8)" --include="*.go" .   # expect: internal/randid + internal/qcow2 only

That last grep is the cheap version of the §4b guard, and its expected output
is only correct because the CLI migration is in scope — if cmd/litevirt/sg.go
still had its generator, this line and the guard would both disagree with the
execution plan. Running it by hand before writing the guard is the quickest way
to notice that class of contradiction.

make ci-guards is expected to pass untouched: no schema change (no
internal/corrosion/schema.go edit, so no CurrentSchemaVersion bump), no SQL
builder change (stmtshapecheck unaffected), no corrosion.<Fn> write-error
handling change (writecheck unaffected), and no new CLI command, config key, or
litevirt_* identifier (docs triangulation unaffected — it only checks those
three things, so a new internal package needs no doc).

---

5. Reviewer notes

Things about this codebase that bear on whether the approach is right.

There is an eighth generator, and the task names seven.
cmd/litevirt/sg.go:244 newID() (string, error) is byte-identical to
newUIID — and it is the origin of the scheme, since newUIID's comment says
it "matches the lv CLI's newID scheme". Its own comment ("We use raw 8-byte
random rather than UUIDs to keep ids short in CLI output") is the best statement
in the repo of why the format is what it is, and worth preserving into
randid's doc comment.

This is now a required step, and it widens the task's stated scope by one
file. Flagging that plainly rather than slipping it in. An earlier draft of
this plan marked it optional while also having the drift guard scan cmd/ —
which was incoherent, since the guard flags this exact function, so step 2 could
never have gone green. Review round 4 caught it. Two ways out, and this is the
better one:

• delete newID (243–250), drop crypto/rand + encoding/hex (sole user);
• sites 92 and 172 become id := randid.New(), and each surrounding
  if err != nil { return err } collapses;
• cmd/litevirt is package main in the same module, so the import is fine.

Beyond making the guard coherent, it is the right call on the merits: leaving it
converts the drift from 7 copies to 2 rather than to 0, keeps a comment
cross-referencing a scheme that no longer exists anywhere else, and leaves the
error policy split after a change whose entire purpose was to unify it. One of
the two surviving copies would be the one the other six were modelled on.

If the scope widening is vetoed — the task did say seven — the fallback is
coherent but weaker, and needs three specific corrections, not just dropping
step 3:

1. scope the guard's walk to internal/ only (drop cmd/ and tests/);
2. change §4b's "No innocent code trips it" to note the guard is deliberately
   blind to cmd/, where a known duplicate survives;
3. add a // TODO at cmd/litevirt/sg.go:243 recording that the generator is a
   known duplicate the guard cannot see, so the next reader isn't misled by a
   green test suite.

I'd rather not: a guard whose blind spot is precisely where the known duplicate
lives teaches the wrong lesson, and this repo's other guards
(writecheck, stmtshapecheck) use explicit per-line opt-outs with stated
reasons instead of silent directory exclusions.

internal/pki is documented as not importable across modules, but this repo is
a single module (one go.mod, verified) — that warning is about external
consumers and does not constrain internal/randid.

These IDs are CRDT primary keys, which is why format stability is a hard
constraint and not a style preference. They key corrosion rows for audit
entries, security groups and rules, firewall rules, IP sets, notification
targets and routes, reservations, LB generations, relocation tokens, and
scheduler proposals. Rows merge last-writer-wins on the PK, so an ID collision
does not error — two logically distinct rows silently become one. 8 bytes is 64
bits; the birthday bound puts collisions in the billions of rows per table.
This plan preserves that entropy exactly. It is also why randid.New must stay
crypto/rand-backed and must never be "optimised" to math/rand: two of the
call sites (internal/failover/coordinator.go:1363,1527) use the value as a
relocation token, not just a name.

No injectable/seedable variant is included, on purpose. Nothing in the repo
stubs ID generation today — the four existing tests only check uniqueness and
length, and tests that need a deterministic ID already pass a literal. Adding a
package-level var reader = rand.Reader seam now would be YAGNI and would add
global mutable state to a leaf package. If a future test genuinely needs
determinism, the fix is to pass the ID in as an argument, which is already the
prevailing style (s.admitResizeReservation(ctx, newID(), ...)).

Why delete the seven wrappers rather than keep them as one-line
delegations. Keeping func newID() string { return randid.New() } in each
package would be a ~10-line diff instead of a ~50-site one, and would
deduplicate the logic. It was rejected: it leaves seven names for one concept
(so the next contributor still can't tell whether generateID and
newNotifyID differ), it leaves newUIID's error return and therefore
mustUIID in place, and it doesn't unify the error policy at the call sites,
which is half of the stated goal. The larger diff is entirely mechanical and
go build catches every miss.

The //nolint:errcheck markers are not load-bearing. There is no
.golangci.yml in this repo and CI runs only go vet ./.... Four sites carry
//nolint:errcheck and two use _, _ = for the same purpose — itself a small
piece of the same drift. All of it converges on one _, _ = line.

---

6. Execution order

Three commits, each independently reviewable and each leaving the tree green.
Conventional-commit style with package scopes, per CLAUDE.md.

The ordering below is a correction from an earlier draft, which shipped the
drift guard in commit 1 and left the CLI migration optional in commit 3. That
could not work: the guard scans cmd/, so it stays red from commit 1 until the
eighth generator is gone (§4b). The guard therefore lands last, when there is
nothing left for it to flag.

1. feat(randid): add shared 8-byte hex random ID helper
internal/randid/randid.go + randid_test.go (the §4a format and uniqueness
   tests only — not the guard). Self-contained and green: it adds a package
   with tests and changes no caller.

2. refactor(randid): replace eight duplicated 8-byte hex ID generators
   The seven named generators plus the CLI's (§5): 20 production files, 4 test
   files, and cmd/litevirt/sg.go. Delete the eight definitions plus
   mustUIID, rewrite the 50 sites (+2 in sg.go), fix imports, delete the two
   UI error branches and the four superseded tests, update the two stale
   comments. Green: go build catches any missed site. Run the full §4e
   sequence here.

3. test(randid): guard against re-introducing local 8-byte hex ID generators
   The §4b guard. Green on arrival, precisely because commit 2 left it nothing to
   find — which is also why the guard's own red-to-green proof has to come from
   the §4c mutation matrix (re-add a local generator, confirm red, revert) rather
   than from commit ordering. Droppable only together with the sg.go part of
   commit 2 (§5).

Then the mutation matrix in §4c, since a green suite here is otherwise
indistinguishable from a suite that stopped testing anything.

If the §5 scope veto applies, commit 2 excludes cmd/litevirt/sg.go and commit
3's guard walks internal/ only — with the two claim corrections and the TODO
listed there.

---

Appendix A: verification log

Added after review round 1, in which the reviewer's sandbox could not read the
workspace at all (see Appendix B) and so
could not check any claim above. Every load-bearing factual claim in this plan,
with the command that establishes it and what it returned in this worktree at
HEAD = de655ea. A reviewer with filesystem access should re-run these rather
than trust the table; a reviewer still without access at least has an auditable
trail.

#	Claim	Command	Result
1	The seven generators exist at the cited lines	grep -n "func newID|func generateID|func newNotifyID|func ownerAssertID|func newUIID" <the 7 files>	coordinator.go:1679, host.go:934, users.go:436, rebalancer.go:709, notifications.go:91, owner_assert.go:335, handle_security_groups.go:173 — all match the task's line numbers
2	Policy split is 6 discard / 1 propagate	read all seven bodies	//nolint:errcheck discard ×4 (coordinator, host, users, owner_assert); _, _ = discard ×2 (rebalancer, notifications); return "", err ×1 (newUIID)
3	crypto/rand.Read cannot fail	go doc crypto/rand Read; $(go env GOROOT)/src/crypto/rand/rand.go:41-46	"It never returns an error, and always fills b entirely… crashes the program irrecoverably"
4	Toolchain is Go 1.26	go version; head go.mod	go1.26.0 linux/amd64; go 1.26.0
5	Single module — no cross-module import problem	find . -name go.mod -not -path "./.git/*"	exactly one: ./go.mod
6	No lint gate; the //nolint markers aren't load-bearing	find . -name ".golangci*"; grep -n "lint|errcheck|vet" .github/workflows/ci.yml	no config file; one hit — run: go vet ./...
7	mustUIID exists purely to discard the error, 3 call sites	grep -rn "mustUIID"	decl at handle_firewall.go:204, callers at 74, 106, 145
8	Site counts: 48 production rewrites in 20 files, 2 test rewrites, 57 references total	grep -rn "\bnewID(|\bgenerateID(|\bnewNotifyID(|\bownerAssertID(|\bnewUIID(|\bmustUIID(" --include="*.go" internal/ | grep -v ":func |//"	57 lines; 49 production (48 rewrites + 1 inside mustUIID), 8 in test files, 20 distinct production files
9	The drift guard has zero false positives	grep -rn "make(\[\]byte, 8)" --include="*.go" .	8 generator sites + internal/qcow2/header.go:232 only; qcow2 imports neither crypto/rand nor encoding/hex (grep -n "crypto/rand|encoding/hex" internal/qcow2/*.go → no production hit)
10	The three excluded generators differ in size, so the size-8 guard needs no allowlist	grep -A2 "func generatePassword|func newSessionID|func randomChunkSuffix"	16, 32, 4 bytes respectively
11	Four superseded tests, at the cited lines	grep -n "^func TestGenerateID\b|^func TestNewID_UniqueAndLength|^func TestGenerateID_Unique|^func TestNewID_Unique" internal/grpcapi/*_test.go	host_test.go:347, users_test.go:190, coverage_test.go:3673, coverage_test.go:3822
12	internal/safename is the stated precedent	head -20 internal/safename/safename.go	"zero-dependency (stdlib only) so any package can import it without risking an import cycle… This generalizes the per-package withinDir/safeJoin helpers"
13	repoRoot walk-up idiom exists to copy	sed -n '585,605p' cmd/litevirt/docs_triangulation_test.go	repoRoot(t) walks up to go.mod, t.Fatal if absent
14	Docs guard can't be tripped by a new internal package	read docs_triangulation_test.go	checks only CLI commands, config keys, and litevirt_* string literals
15	writecheck can't be tripped	head -40 scripts/ci/writecheck/main.go	flags only discarded returns from a closed set of corrosion.<Fn> writers
16	The eighth generator exists and is the scheme's origin	sed -n '243,250p' cmd/litevirt/sg.go	func newID() (string, error), comment: "raw 8-byte random rather than UUIDs to keep ids short in CLI output"
17	Deleting the UI error branches breaks no test	grep -rln "id generation failed" internal/ui/	only the two handlers; no _test.go hit
18	The eighth generator matches all three guard predicates in one body, so the guard cannot be green while it exists	sed -n '243,251p' cmd/litevirt/sg.go	make([]byte, 8), rand.Read(b), hex.EncodeToString(b) — all present in newID

Two things this log does not establish, because they can only be checked
after the code is written: that go build ./... && go vet ./... && go test ./...
stay green, and that the §4c mutations actually turn the new tests red. Those are
implementation-time obligations, not plan claims.

Corrections this plan has been through, recorded so a reviewer can see what
moved rather than diffing drafts:

• Round 2 (self-caught while building this table): an earlier draft said "55
  call sites." Recounting produced 57 references / 50 rewrites. The per-file
  tables in §3 were correct — only the summary figure was wrong.
• Round 4 (reviewer-caught, claim 18 above): the drift guard scanned cmd/
  while the CLI migration was marked optional, so the guard could never have gone
  green at the end of commit 2 as §6 claimed. Resolved by making the CLI
  migration required (§5), moving the guard to the last commit (§6), and
  restating §4b's no-allowlist claim in terms of innocent code rather than
  implying nothing matches. This was a real internal contradiction, not a
  presentation issue: following the old §6 literally would have left CI red.

---

Appendix B: reviewer sandbox failure (rounds 1–3)

Three consecutive review rounds returned the same single finding — escalating
from major to critical — that PLAN.md and the referenced source files could
not be read because the sandbox launcher could not create its namespace, so the
plan could not be validated.

I don't think this is a defect in the plan, and I've changed nothing in
response to it beyond adding Appendix A. It reports that the reviewer's
sandbox could not start, not that anything in the plan is wrong. There is no
edit to PLAN.md that would fix it. Recording the diagnosis here since I could
reproduce it and the next reviewer will likely hit the same wall.

I agree with the finding's conclusion, though, and want to be unambiguous
about it: do not treat this plan as reviewed. Three failed rounds are three
non-reviews, not tacit approval, and Appendix A is self-verification, which is
not independent verification. The plan should not be implemented on the strength
of these rounds.

The artifact is present and readable. PLAN.md, mode -rw-rw-r--, owner
kal:kal, in the worktree root at
/home/kal/.overseer/worktrees/replace-the-seven-duplicated-8-byte-hex-random-id-generators/PLAN.md.
My own tools read it and the whole repo without trouble in this same session.

Reproduced, with a root cause. bwrap --dev-bind / / --unshare-pid true
fails here with "No permissions to create a new namespace, likely because the
kernel does not allow non-privileged user namespaces." The usual suspects are
not the cause — /proc/sys/kernel/unprivileged_userns_clone is 1 and
/proc/sys/user/max_user_namespaces is 2147483647. The cause is:

/proc/sys/kernel/apparmor_restrict_unprivileged_userns = 1

On this kernel (Linux 7.0.0-28-generic) that AppArmor setting blocks
unprivileged user-namespace creation for unconfined binaries, which is exactly
what bwrap 0.11.1 needs. Resolution is environmental and needs root, so it is
the operator's call, not something I should change unasked — a system-wide
sandboxing sysctl is not mine to flip. The options, in order of preference:

1. Run the review with its sandbox disabled — the reviewer only needs read
   access, and the finding says reads are what failed.
2. Install an AppArmor profile for /usr/bin/bwrap granting userns create
   (the packaged fix on distros that ship this restriction).
3. sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 — effective
   but it lowers a host-wide security boundary; least preferred.

Re-checked at round 3: bwrap fails identically and the sysctl is still 1.
Nothing has changed, so nothing I can do from inside the workspace will change
the outcome of round 4 either.

The one alternative hypothesis is now ruled out. PLAN.md is gitignored
(.gitignore:71:/PLAN.md, confirmed via git check-ignore -v PLAN.md), and
review tooling that enumerates tracked files would miss it for that reason
alone. But rounds 2 and 3 report that the referenced source files were
unreadable too, and those are tracked — git ls-files --error-unmatch internal/failover/coordinator.go succeeds. A gitignore rule cannot hide a
tracked file. So the failure is the namespace error at the read layer, affecting
every path equally, and git add -f PLAN.md would not help. I have therefore
not done it: it would not fix the finding, and committing an artifact the task
scoped as the sole deliverable is the operator's decision, not mine.

What I am asking for, since I cannot do it myself. Every remedy needs root
on the host. Preference order is above; option 1 (run the reviewer without its
sandbox — it only needs read access, and reads are precisely what failed) is
both the safest and the most likely to be a one-line config change on the review
harness. Option 3 flips a host-wide AppArmor boundary and I would not run it
unasked even if I could.

Meanwhile, the substance is reviewable without filesystem access. This plan
quotes inline every source excerpt its argument depends on — the crypto/rand
contract (§1a), the mustUIID comment (§1b), the safename precedent (§2), the
UI error branch being deleted (§3) — and Appendix A pairs each factual claim
with the command that establishes it. A reviewer who still cannot read the tree
can judge the design decisions from the plan text alone, and flag any claim in
Appendix A they want re-run once reads work.

---

Appendix C: implementation log — deviations from the plan

Written after carrying the plan out. The plan was accurate: every line number,
site count, import-drop verdict, and Appendix A claim I re-checked held at
HEAD = de655ea. Four things differ from what §3–§6 specify.

1. One working tree, not three commits (§6). The implementation task
explicitly said not to commit. All three of §6's steps are present as a single
uncommitted change. The intermediate green states §6 relied on were still
verified in order: internal/randid built and tested green before any caller
moved, and the drift guard was written only after the eighth generator was
gone, so it has never been red on arrival.

2. A third stale comment, not the two §3 names.
internal/corrosion/action_proofs.go:217 read "A relocation token is
single-mint (newID per relocation)" — a reference to failover.newID, which
this change deletes. §3 lists only reservation_admission_test.go:102 and
overcommit_reservation_test.go:21. Updated all three to say randid.New(),
for the reason §3 gives for the other two: don't send a future reader looking
for a symbol that no longer exists. internal/corrosion/vms.go:959 also matches
a newID grep but is an unrelated local variable, left alone.

Consequence for §3's file count: 22 production files change, not the 20 §3
tables list plus cmd/litevirt/sg.go. The extra one is this comment.

3. The guard's "scan path wrong?" self-check is stricter than §4c asked
for. §4c's fifth mutation only requires that pointing the scan at a
nonexistent subdirectory fails loudly rather than passing on zero files. The
guard does that two ways: an os.Stat per scan root that t.Fatals by name,
and the scanned == 0 assertion §4b specified. The os.Stat leg is the one
that fires for a mistyped directory; the count leg still covers a root that
exists but yields no .go files. Both are cheap and they fail for different
reasons, so I kept both. Mutation 5 was verified against the os.Stat leg.

4. Pre-existing gofmt drift left alone. gofmt -l internal/grpcapi/
reports eight files (dualrun.go, move.go, and six _test.go). All eight are
untouched by this change — confirmed with git diff --quiet HEAD -- <file> on
each — so the drift predates it and reformatting them would be unrelated noise
in this diff. Every file this change does touch is gofmt-clean.

Verification actually run

Command	Result
go build ./... && go vet ./...	clean
go test ./...	exit 0, 63 packages ok, including tests/fleet and cmd/litevirt
make ci-guards	exit 0 — schema-bump, ledger-drift, writecheck, stmtshapecheck, docs triangulation all OK, as §4e predicted
grep -rn "make(\[\]byte, 8)"	internal/qcow2/header.go:232 plus three occurrences inside the guard's own comments — no generator survives anywhere

All five §4c mutations were applied, confirmed red, and reverted:

Mutation	Observed failure
Bytes = 4	all three named tests failed (decodes to 4 bytes, want 8; "74ec0286" vs the 16-char regexp; Bytes = 4, want 8)
delete the rand.Read line	TestNewUnique: collision after 1 draws: "0000000000000000"
hex → base64.StdEncoding	TestNewLengthAndAlphabet: New() = "qaPfQVkehWM="
paste the old ownerAssertID back into internal/health/owner_assert.go	TestNoLocalEightByteHexIDGenerators: internal/health/owner_assert.go:336 func ownerAssertID
point the guard's scan at internal-typo	scan dir …/internal-typo missing: … — scan path wrong?

Nothing in the plan turned out to be wrong about the code.

Final Codex review

No blocking findings remained.

---

Opened by overseer. The plan and the code each converged to zero blocking findings from Codex before this pull request was created.