feat(encryption): foundation package (Stage 0)

[bootjp]

Summary

Stage 0 of the data-at-rest encryption rollout from PR #707 (docs/design/2026_04_29_proposed_data_at_rest_encryption.md). This PR is library-only — no integration with existing storage / raft / FSM layers. Those land in Stages 1–9.

New package internal/encryption/:

• cipher.go — AES-256-GCM Encrypt / Decrypt primitive over a Keystore. Caller-supplied AAD bytes (no AAD composition baked in — storage and raft layers compose their own AAD per §4.1 / §4.2 in later stages). Decrypt wraps GCM tag mismatch as ErrIntegrity with the original Open error attached as a secondary cause, so callers match via errors.Is and never silently zero or retry.
• envelope.go — §4.1 wire format encoder/decoder. Layout: [version 1B][flag 1B][key_id 4B BE][nonce 12B][ciphertext+tag]. EnvelopeOverhead = 34 bytes. HeaderAADBytes helper exposed for storage callers that compose AAD = HeaderAADBytes ‖ pebble_key. ReservedKeyID = 0 sentinel per §5.1.
• keystore.go — copy-on-write atomic.Pointer[map[uint32][]byte] DEK store. Get is a single atomic load (no mutex on the hot path per §10 self-review lens 2). Set rejects ReservedKeyID and non-32-byte DEKs; copies input.
• errors.go — typed errors: ErrUnknownKeyID, ErrReservedKeyID, ErrBadNonceSize, ErrBadKeySize, ErrIntegrity, ErrEnvelopeShort, ErrEnvelopeVersion.
• kek/kek.go — Wrapper interface (Wrap / Unwrap / Name). KMS-backed providers (AWS KMS, GCP KMS, Vault) come in Stage 9.
• kek/file.go — FileWrapper: AES-256-GCM under a 32-byte KEK read from a file. Output [12B nonce][32B ct][16B tag] = 60 bytes. Validates lengths instead of padding/truncating.

Tests:

• cipher_test.go — round-trip across plaintext/AAD shapes; tag/AAD/ciphertext/nonce tamper rejected with ErrIntegrity; typed error checks for reserved/bad-nonce/unknown-key cases; distinct nonces produce distinct ciphertexts.
• cipher_prop_test.go — pgregory.net/rapid property tests for arbitrary plaintext/AAD round-trip and single-bit AAD-flip rejection (the §4.1 cut-and-paste defence property).
• envelope_test.go — encode/decode round-trip; under-length input rejected; unknown version bytes rejected; decode does not alias input.
• keystore_test.go — Set/Get/Delete/IDs/Len; concurrent reader/writer stress under -race.
• kek/file_test.go — round-trip; distinct Wrap nonces; bad-length rejection; tag/nonce tamper rejected; missing file.

Self-review (per CLAUDE.md 5 lenses)

1. Data loss — Pure library, no persistence yet. ErrIntegrity always propagated. Round-trip property test guards envelope-format bugs.
2. Concurrency — atomic.Pointer[map] for keystore; concurrent stress test under -race.
3. Performance — AES-NI via crypto/aes. No allocations beyond per-call ciphertext buffer (sync.Pool optimisation deferred to integration stages).
4. Data consistency — No MVCC/OCC interaction yet.
5. Test coverage — 23 unit tests + 2 property tests, -race clean, 0 lint issues against project .golangci.yaml.

Test plan

• go test ./internal/encryption/... (~14s)
• go test -race ./internal/encryption/... (~65s)
• golangci-lint run ./internal/encryption/... (0 issues)
• Reviewer: confirm Cipher API does not bake storage/raft AAD assumptions in (§4.1 vs §4.2 layouts must remain orthogonal).
• Reviewer: confirm ReservedKeyID = 0 is rejected at all entry points (Cipher.Encrypt, Cipher.Decrypt, Keystore.Set).

[coderabbitai]

Warning

Rate limit exceeded

@bootjp has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 33 minutes and 37 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bbec81d0-9959-45cd-80d8-dbc034c9ac50

📥 Commits

Reviewing files that changed from the base of the PR and between 2dda2cb and c3f41a4.

📒 Files selected for processing (13)

• internal/encryption/cipher.go
• internal/encryption/cipher_bench_test.go
• internal/encryption/cipher_prop_test.go
• internal/encryption/cipher_test.go
• internal/encryption/doc.go
• internal/encryption/envelope.go
• internal/encryption/envelope_test.go
• internal/encryption/errors.go
• internal/encryption/kek/file.go
• internal/encryption/kek/file_test.go
• internal/encryption/kek/kek.go
• internal/encryption/keystore.go
• internal/encryption/keystore_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/encryption-foundation

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 0/1 reviews remaining, refill in 33 minutes and 37 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 3m 1s —— View job

---

Code Review: feat/encryption-foundation (Stage 0)

• Gather context and read changed files
• Review errors.go and keystore.go
• Review cipher.go and envelope.go
• Review kek/kek.go and kek/file.go
• Review test files
• Compile findings and post review

---

Overall this is a solid foundation. The architecture is clean, the copy-on-write keystore is correctly implemented, error propagation follows project conventions, and the test coverage is thorough. A few items need addressing before merge, ranging from a missing security warning down to minor style nits.

---

Must Fix

1. Nonce reuse not documented in Cipher.Encrypt

cipher.go:35 — The doc comment for Encrypt lists three constraints but is silent on nonce uniqueness. For AES-GCM, reusing the same (key, nonce) pair with any two distinct plaintexts leaks the XOR of the plaintexts and enables key recovery in some constructions. This is the most catastrophic misuse of GCM and belongs prominently in the API doc.

// CRITICAL: callers MUST NOT reuse the same nonce with the same keyID.
// Nonce reuse under GCM is catastrophic — it leaks plaintext XOR and
// enables keystream recovery. Generate nonces with crypto/rand or a
// per-key monotonic counter.

Fix this →

---

2. Keystore.Get exposes a mutable slice into the live DEK map

keystore.go:36-40 — Get returns the exact []byte stored inside the map snapshot. A caller who violates the "MUST NOT be modified" contract silently corrupts the active DEK bytes, because the next Set CAS creates a new map but copies the existing DEK slice references (not the values) when building the new map copy (keystore.go:66). The corruption persists until the key is explicitly overwritten.

For a crypto-critical path, a defensive copy in Get is warranted. The cost is exactly 32 bytes per encrypt/decrypt — negligible compared to the AES-GCM operation itself.

func (k *Keystore) Get(keyID uint32) ([]byte, bool) {
    m := *k.snap.Load()
    dek, ok := m[keyID]
    if !ok {
        return nil, false
    }
    cp := make([]byte, len(dek))
    copy(cp, dek)
    return cp, true
}

Alternatively, store [KeySize]byte values in the map instead of slices to make the type system enforce immutability.

Fix this →

---

3. Missing tests for Decrypt rejecting ReservedKeyID and bad nonce size

cipher_test.go has TestCipher_Encrypt_RejectsReservedKeyID and TestCipher_Encrypt_RejectsBadNonceSize, but the PR's own test-plan checklist says:

Reviewer: confirm ReservedKeyID = 0 is rejected at all entry points

Both Decrypt paths go through aeadFor which does check these, so the code is correct — but the tests don't cover the Decrypt direction. Add:

func TestCipher_Decrypt_RejectsReservedKeyID(t *testing.T) { ... }
func TestCipher_Decrypt_RejectsBadNonceSize(t *testing.T) { ... }

Fix this →

---

Should Fix

4. EnvelopeVersionV1 comment is misleading about forward-compatibility

envelope.go:12-14:

// §11.3 reserves 0x02..0x0F for future authenticated formats; values
// outside that range cause DecodeEnvelope to return ErrEnvelopeVersion.

"Values outside that range" implies 0x02..0x0F are accepted (or at least treated differently), but the implementation rejects any byte that is not 0x01. Values 0x02..0x0F are currently indistinguishable from 0xFF to the decoder — all produce ErrEnvelopeVersion. If §11.3 genuinely reserves that range for forward-compatibility, the implementation should document the intent more clearly:

// §11.3 reserves 0x02..0x0F for future authenticated formats. The
// current build only understands 0x01; any other version byte causes
// DecodeEnvelope to return ErrEnvelopeVersion. Future decoders that
// know how to handle 0x02..0x0F will update this check.

Fix this →

---

5. Envelope.Encode() produces undecodable envelopes silently

envelope.go:77 — Encode() has no guard for e.Version != EnvelopeVersionV1 or len(e.Body) < TagSize. An envelope built with the wrong version byte or a too-short body will encode fine and then cause DecodeEnvelope or Cipher.Decrypt to fail later, making the root cause hard to trace. Add validation at Encode time — the storage layer in Stage 1 will be constructing Envelope values and a fast-fail here is safer:

func (e *Envelope) Encode() ([]byte, error) {
    if e.Version != EnvelopeVersionV1 {
        return nil, errors.Wrapf(ErrEnvelopeVersion, "encode: got 0x%02x", e.Version)
    }
    if len(e.Body) < TagSize {
        return nil, errors.Wrapf(ErrEnvelopeShort, "encode: body %d bytes, want >= %d", len(e.Body), TagSize)
    }
    // ...
}

(This also changes the return signature — callers in later stages should handle the error.)

Fix this →

---

Minor / Style

6. sort.Slice → slices.Sort in keystore.go:102

The module is at Go 1.25 — slices.Sort (golang.org/x/exp/slices since 1.21, stdlib since 1.21) is idiomatic and avoids the closure allocation:

import "slices"
// ...
slices.Sort(ids)

Fix this →

---

7. FileWrapper.path field is stored but never read

kek/file.go:33 — path is stored at construction time (NewFileWrapper) but is never referenced again — not in Wrap, Unwrap, or Name. If it's kept for future diagnostic use, document why; otherwise remove it to avoid confusion. At minimum Name() could return "file:"+w.path to make it actionable in logs.

Fix this →

---

8. Unreachable post-Open length check in FileWrapper.Unwrap

kek/file.go:95-98:

if len(plain) != fileKEKSize {
    return nil, errors.Errorf(...)
}

aead.Open on a fileKEKSize-byte ciphertext (= len(ct) - TagSize = 32+16 - 16 = 32) always yields exactly fileKEKSize bytes on success; the preceding strict-length check on wrapped (line 85) guarantees len(ct) == fileKEKSize+fileTagSize. This branch can never fire. Remove it, or move the check to document the invariant as a comment.

---

9. Per-call AEAD allocation in Cipher.aeadFor (acknowledged, track it)

cipher.go:83-91 — Every Encrypt/Decrypt creates a fresh aes.Block + cipher.AEAD. The PR notes this as deferred to integration stages. A sync.Pool of pre-keyed AEADs keyed by keyID is the standard fix. Confirm this is tracked in the Stage 1 or later design doc so it doesn't get lost.

---

Confirmed (Reviewer Checklist Items)

• Cipher API does not bake storage/raft AAD — Confirmed. Encrypt/Decrypt accept raw aad []byte; composition of HeaderAADBytes ‖ pebble_key (§4.1) and "R" ‖ version ‖ key_id (§4.2) is left entirely to callers. The primitive is orthogonal. ✅
• ReservedKeyID = 0 rejected at all entry points — Code-confirmed via aeadFor for Cipher.Encrypt/Decrypt, and the Set guard in Keystore. Tests only verify the Encrypt path (see item 3 above). ⚠️

[gemini-code-assist]

Code Review

This pull request implements the foundation for AES-256-GCM envelope encryption, including a thread-safe copy-on-write Keystore for Data Encryption Keys (DEKs), a Cipher for cryptographic operations, and wire format encoding/decoding. The review feedback focuses on performance optimizations for the hot path, specifically recommending the caching of cipher.AEAD instances to avoid repeated initialization overhead and suggesting an alternative API for AAD byte generation to reduce GC pressure.

[gemini-code-assist]

The aeadFor method performs AES key expansion (aes.NewCipher) and GCM initialization (cipher.NewGCM) on every encryption and decryption operation. These are computationally expensive operations that will significantly degrade performance on the hot path. Since Data Encryption Keys (DEKs) are relatively stable, the cipher.AEAD instances should be pre-computed and cached (e.g., within the Keystore) to avoid this overhead.

References

1. Optimizing the hot path by caching expensive cryptographic initializations aligns with repository performance standards for frequently accessed code paths.

[gemini-code-assist]

To improve performance on the hot path (encryption/decryption), the Keystore should store pre-initialized cipher.AEAD instances instead of just raw DEK bytes. This avoids repeated key expansion and GCM setup costs. Consider changing the internal map to store a struct containing both the raw DEK and the initialized cipher.AEAD object.

References

1. Pre-computing and caching AEAD instances avoids repeated overhead on hot paths, consistent with repository performance optimization guidelines.

[gemini-code-assist]

This function allocates a new byte slice on every call, which can lead to significant GC pressure in high-frequency operations. Consider an alternative API that allows the caller to pass a pre-allocated buffer or appends to an existing slice. When doing so, ensure a strictly enforced maximum size limit is checked to prevent unbounded memory growth, consistent with repository performance and safety rules.

Suggested change

	func HeaderAADBytes(version, flag byte, keyID uint32) []byte {
	out := make([]byte, HeaderAADSize)
	out[0] = version
	out[1] = flag
	binary.BigEndian.PutUint32(out[2:HeaderAADSize], keyID)
	return out
	}
	func AppendHeaderAADBytes(dst []byte, version, flag byte, keyID uint32) []byte {
	out := [HeaderAADSize]byte{
	version,
	flag,
	}
	binary.BigEndian.PutUint32(out[2:], keyID)
	return append(dst, out[:]...)
	}

References

1. Pre-allocating buffers is acceptable and preferred for performance when strictly enforced maximum size limits are checked before allocation to prevent OOM issues.

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 2m 38s —— View job

---

Code Review: feat/encryption-foundation (Stage 0)

• Read all changed files
• Review errors.go and keystore.go
• Review cipher.go and envelope.go
• Review kek/kek.go and kek/file.go
• Review test files
• Compile findings and post review

---

The package is well-structured, the copy-on-write keystore is correctly implemented, error propagation follows project conventions, and the property tests are a strong safety net. A few items need addressing before merge.

---

Must Fix

1. Cipher.Encrypt doc is silent on nonce reuse — the most catastrophic GCM misuse

cipher.go:25-34 — The constraints listed in the doc comment cover ReservedKeyID, nonce size, and key presence, but say nothing about nonce uniqueness. Reusing the same (keyID, nonce) pair on two distinct plaintexts leaks the XOR of those plaintexts and enables keystream recovery under GCM. This must be front-and-centre in the API doc before callers in store/ and raftengine/ compose nonces in later stages.

// CRITICAL: callers MUST NOT reuse the same (keyID, nonce) pair for
// different plaintexts. GCM nonce reuse leaks plaintext XOR and enables
// keystream recovery. Generate nonces from crypto/rand or a per-key
// monotonic counter.

Fix this →

---

2. Keystore.Get exposes the internal DEK slice to mutation

keystore.go:36-39 — Get returns the exact []byte stored inside the live map snapshot. The doc says "MUST NOT be modified", but the Set CAS loop copies the map structure, not the slice values (keystore.go:66: m[id] = v). A caller that violates the contract silently corrupts the active DEK, and the corruption persists across rotations until the key is explicitly overwritten. For a crypto-critical path, a 32-byte defensive copy in Get is negligible overhead.

func (k *Keystore) Get(keyID uint32) ([]byte, bool) {
    m := *k.snap.Load()
    dek, ok := m[keyID]
    if !ok {
        return nil, false
    }
    cp := make([]byte, len(dek))
    copy(cp, dek)
    return cp, true
}

Related: TestKeystore_Set_CopiesInput verifies that mutating the input to Set doesn't affect stored data, but there's no corresponding test that mutating the output of Get doesn't corrupt the store. A TestKeystore_Get_DoesNotExposeInternalSlice test would lock this down:

got, _ := ks.Get(1)
got[0] ^= 0xFF
got2, _ := ks.Get(1)
if got2[0] != 0xAA { // original value
    t.Fatal("Get returned aliased internal slice")
}

Fix this →

---

3. Missing Decrypt path tests for ReservedKeyID and ErrBadNonceSize

cipher_test.go has TestCipher_Encrypt_RejectsReservedKeyID and TestCipher_Encrypt_RejectsBadNonceSize but no matching Decrypt variants. Both paths go through aeadFor (which does check these), so the code is correct — but the PR's own test-plan checklist says:

Reviewer: confirm ReservedKeyID = 0 is rejected at all entry points

Without Decrypt-direction tests this checklist item cannot be ticked. Add:

func TestCipher_Decrypt_RejectsReservedKeyID(t *testing.T) { ... }
func TestCipher_Decrypt_RejectsBadNonceSize(t *testing.T)  { ... }

Fix this →

---

Should Fix

4. Envelope.Encode() silently serialises invalid envelopes

envelope.go:77 — Encode has no validation guard. An Envelope built with Version != EnvelopeVersionV1 or len(Body) < TagSize will encode fine and then fail opaquely at DecodeEnvelope or Cipher.Decrypt in Stage 1+. Fast-fail here is safer since store/ callers will construct Envelope values directly. Note this requires a signature change from []byte to ([]byte, error) to propagate the validation.

Fix this →

---

5. EnvelopeVersionV1 comment is misleading about what DecodeEnvelope accepts

envelope.go:12-14:

§11.3 reserves 0x02..0x0F for future authenticated formats; values outside that range cause DecodeEnvelope to return ErrEnvelopeVersion.

"Values outside that range" implies 0x02..0x0F are treated specially, but the implementation rejects any byte that isn't 0x01. Replace with:

// §11.3 reserves 0x02..0x0F for future authenticated formats. The
// current build only recognises 0x01; any other version byte causes
// DecodeEnvelope to return ErrEnvelopeVersion. Future decoders that
// understand 0x02..0x0F will widen this check.

Fix this →

---

6. No benchmark for the Cipher hot path

cipher.go:83-91 — Every Encrypt/Decrypt call runs aes.NewCipher + cipher.NewGCM (key expansion + AEAD setup). The PR acknowledges this and defers the sync.Pool optimisation to later stages. However, there's no baseline benchmark to anchor future optimisation work. Per CLAUDE.md: "add one if a hot path changed." Noting that FileWrapper already demonstrates the right pattern — it caches the AEAD at construction time (kek/file.go:33). A future Cipher refactor could follow the same approach by caching per-keyID AEADs in the Keystore.

Recommend adding BenchmarkCipher_Encrypt and BenchmarkCipher_Decrypt before merge so the Stage 1 integration has a baseline to compare against.

Fix this →

---

Minor / Style

7. sort.Slice → slices.Sort in keystore.go:102

Go 1.21+ stdlib; the module is at Go 1.25. Avoids the closure allocation and is idiomatic:

import "slices"
slices.Sort(ids)

Fix this →

---

8. FileWrapper.path stored but never used

kek/file.go:33-35 — path is captured at construction time but referenced nowhere after that — not in Wrap, Unwrap, or Name. At minimum Name() should return "file:"+w.path so operator logs and the EncryptionAdmin status RPC (Stage 9) can identify which key file is loaded. Otherwise remove the field.

Fix this →

---

9. Unreachable post-Open length check in FileWrapper.Unwrap

kek/file.go:95-98 — The strict-length guard on wrapped at line 85 guarantees len(ct) == fileKEKSize + fileTagSize going into aead.Open. On success Open always returns exactly fileKEKSize bytes. The len(plain) != fileKEKSize branch at line 95 can never fire. Remove it (or convert to a panic("unreachable") if you want to document the invariant).

---

10. FileWrapper.Wrap uses no AAD

kek/file.go:76 — aead.Seal(out, nonce, dek, nil) passes nil AAD. This means a wrapped DEK could theoretically be swapped between key slots at the KEK layer without detection (the key ID isn't bound to the wrapped blob). This is likely an intentional Stage 0 simplification — the key-management RPC context (Stage 9) is the natural place to add AAD binding. Worth an explicit comment noting the deferred scope:

// AAD is nil here; key-ID binding at the KEK layer is deferred to Stage 9
// when the EncryptionAdmin RPC context is available.

---

Confirmed Checklist Items

• Cipher API does not bake storage/raft AAD assumptions — Confirmed. Encrypt/Decrypt accept raw aad []byte; composition of HeaderAADBytes ‖ pebble_key (§4.1) and "R" ‖ version ‖ key_id (§4.2) is left entirely to callers. Orthogonal. ✅
• ReservedKeyID = 0 rejected at all code entry points — Confirmed via aeadFor for Cipher.Encrypt/Decrypt and the Set guard in Keystore. Test coverage is incomplete for the Decrypt direction (see item 3 above). ⚠️

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 12f6dfc9e0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Reject nil keystore when constructing a Cipher

NewCipher currently accepts a nil *Keystore, but the first Encrypt/Decrypt call will panic when aeadFor dereferences c.keystore. This turns a configuration/setup mistake into a process crash instead of a typed error, which is risky for production startup and rotation paths where dependencies may be wired dynamically.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Make zero-value Keystore safe before dereferencing snap

All accessors dereference *k.snap.Load() without checking whether snap was initialized, so a zero-value Keystore (var ks Keystore or embedded struct) panics on first use. Because Keystore is exported, this is an easy misuse pattern and can crash callers outside tests; initializing lazily or guarding nil would avoid that failure mode.

Useful? React with 👍 / 👎.

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 2m 34s —— View job

---

Code Review: feat/encryption-foundation (Stage 0) — Round 3

• Gather context from prior reviews
• Read all changed files in current state
• Review errors.go and keystore.go
• Review cipher.go and envelope.go
• Review kek/kek.go and kek/file.go
• Review test files
• Compile findings and post review

---

All items from the first two review rounds have been addressed. The keystore redesign (pre-initialized AEADs stored as keyEntry, [KeySize]byte array for DEK immutability, AEAD() / DEK() split) is significantly better than a simple defensive copy — it collapses two prior must-fix items and gains a performance win at the same time. Encode() error return, nonce-reuse doc, clarified version comment, AppendHeaderAADBytes, and the Decrypt-path tests are all clean. Four items remain.

---

Must Fix

1. Comments in keystore.go cite the review provenance — violates CLAUDE.md

keystore.go:16 and keystore.go:21:

// production-realistic write rate (gemini-code-assist review on PR #719).
// ...
// hazard flagged by claude[bot] review.

CLAUDE.md is explicit: "Don't reference the current task, fix, or callers...since those belong in the PR description and rot as the codebase evolves." These parentheticals will be meaningless after merge. The WHY is still valuable — drop only the provenance:

// keyEntry holds a single DEK plus its pre-initialized AEAD instance. The
// AEAD is built once at Set time so the hot path (Cipher.Encrypt /
// Cipher.Decrypt) never pays for aes.NewCipher / cipher.NewGCM on every call.
//
// dek is stored as a fixed-size array rather than a slice so the type
// system enforces immutability: callers of DEK() receive a value copy,
// not a pointer into the live map.

Fix this →

---

Should Fix

2. Zero-value Keystore panics on any method call

The codex review flagged this and it is still unaddressed. A zero-value Keystore (via var ks Keystore, struct embedding, or zero-initialisation in a test helper) panics at *k.snap.Load() because the atomic.Pointer starts nil. Since the type is exported and its zero value looks valid, this is a hidden footgun.

The minimal fix is a prominent type-level doc note:

// Keystore is a copy-on-write map from key_id to (DEK, pre-init AEAD).
//
// The zero value is NOT safe to use. Create a Keystore with NewKeystore.
// Embedding a Keystore in another struct requires calling NewKeystore
// explicitly before first use.

A stronger fix is lazy initialization in snap.Load() wrappers, but that adds complexity. Documenting the invariant clearly is a reasonable Stage 0 choice — just make it hard to miss.

Fix this →

---

3. NewCipher(nil) panics at the call site rather than returning a typed error

cipher.go:24-26 — NewCipher does not validate ks != nil. Passing nil causes a nil-dereference panic inside aeadFor at c.keystore.AEAD(keyID), which is hard to trace at the point of misconfiguration (e.g., wiring the Cipher before the Keystore is initialised during startup). A panic at construction time with a clear message is better:

func NewCipher(ks *Keystore) *Cipher {
    if ks == nil {
        panic("encryption.NewCipher: ks must not be nil")
    }
    return &Cipher{keystore: ks}
}

Fix this →

---

4. No benchmark for the now-optimized hot path

The keystore redesign moved AEAD pre-initialization from per-call (in the old aeadFor) to once-at-Set-time. This is the main performance win in this revision. CLAUDE.md states: "add [a benchmark] if a hot path changed." Without a baseline now, the Stage 1 integration has no reference point to verify the optimization held up under realistic usage.

Recommend BenchmarkCipher_Encrypt and BenchmarkCipher_Decrypt covering both small and large plaintexts. The property that a single Set amortises across all subsequent Encrypt/Decrypt calls should be visible in the numbers.

Fix this →

---

Minor

5. FileWrapper.Wrap passes nil AAD with no explanation

kek/file.go:76 — w.aead.Seal(out, nonce, dek, nil). Previous review flagged this as needing a comment documenting the intentional Stage 0 deferral. Without it, a future maintainer adding key-ID binding might add AAD here in isolation and silently break the wrapped-DEK format for existing persisted blobs. One line suffices:

// AAD is nil: key-ID binding at the KEK layer is deferred to Stage 9.
out = w.aead.Seal(out, nonce, dek, nil)

Fix this →

---

Confirmed Improvements Since Last Review

• Pre-initialized AEADs (keyEntry.aead) — eliminates aes.NewCipher + cipher.NewGCM on the hot path. ✅
• [KeySize]byte array for DEK — type-system enforcement of immutability; DEK() returns a value copy. ✅ (TestKeystore_DEK_ValueCopy locks it down)
• TestCipher_Decrypt_RejectsReservedKeyID / TestCipher_Decrypt_RejectsBadNonceSize — Decrypt-path entry-point coverage complete. ✅
• Envelope.Encode() returns ([]byte, error) with version + body-length validation. ✅
• AppendHeaderAADBytes for allocation-free AAD composition on the hot path. ✅
• FileWrapper.Name() returns "file:"+w.path. ✅
• Unreachable post-Open length check removed with doc comment in Unwrap. ✅

---

Reviewer Checklist

• Cipher API does not bake storage/raft AAD — Confirmed. Encrypt/Decrypt accept raw aad []byte; §4.1 and §4.2 composition is left entirely to callers. ✅
• ReservedKeyID = 0 rejected at all entry points — Confirmed via aeadFor (both Encrypt and Decrypt directions now tested), and the Set guard in Keystore. ✅

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 2m 39s —— View job

---

Code Review: feat/encryption-foundation (Stage 0) — Round 4

• Gather context from prior reviews
• Read current state of all changed files
• Review keystore.go and cipher.go
• Review envelope.go and errors.go
• Review kek/file.go and kek/kek.go
• Review test files
• Compile findings and post review

---

Good progress since Round 3. The NewCipher(nil) change (returning (*Cipher, error) + ErrNilKeystore) is a clean solution — the detailed comment explaining the rationale at cipher.go:24-30 is exactly right. Four items from Round 3 remain open and one new stale-comment was found.

---

Must Fix

1. Review-provenance parentheticals still in keystore.go — violates CLAUDE.md

keystore.go:16 and keystore.go:21 still contain:

// production-realistic write rate (gemini-code-assist review on PR #719).
// ...
// hazard flagged by claude[bot] review.

CLAUDE.md is unambiguous: "Don't reference the current task, fix, or callers…since those belong in the PR description and rot as the codebase evolves." These parentheticals are the only Must Fix carried over from the first two rounds that has not landed yet. The WHY is still worth keeping; just drop the provenance:

// keyEntry holds a single DEK plus its pre-initialized AEAD instance.
// The AEAD is built once at Set time so the hot path (Cipher.Encrypt /
// Cipher.Decrypt) never pays for aes.NewCipher / cipher.NewGCM per call.
//
// dek is stored as a fixed-size array so the type system enforces
// immutability: callers of DEK() receive a value copy, not a pointer
// into the live map.

Fix this →

---

Should Fix

2. Keystore type still has no "zero value is not safe" warning

keystore.go:27-34 — The type-level doc says nothing about safe construction. A zero-value Keystore (var ks encryption.Keystore, struct embedding without a NewKeystore() call) will panic at *k.snap.Load() on any method call because the atomic.Pointer starts nil. Since the type is exported and its struct fields are unexported (so callers cannot inspect what's missing), this is an invisible footgun.

// Keystore is a copy-on-write map from key_id to (DEK, pre-init AEAD).
//
// The zero value is NOT safe to use. Create a Keystore with NewKeystore.
// Embedding a Keystore in another struct requires calling NewKeystore
// explicitly before first use.
//
// Reads on the hot path take a single atomic load and observe an immutable
// snapshot of the map. Writes allocate a new map and CAS it in via
// atomic.Pointer.Store.

Fix this →

---

3. Still no benchmark for the pre-initialized AEAD hot path

The keystore redesign (pre-built keyEntry.aead) eliminated per-call aes.NewCipher + cipher.NewGCM. That is the main measurable performance win in this PR. CLAUDE.md states: "add [a benchmark] if a hot path changed." Without a baseline now, Stage 1 integration has nothing to compare against.

Recommend adding BenchmarkCipher_Encrypt and BenchmarkCipher_Decrypt (small + large payload) to cipher_test.go before merge:

func BenchmarkCipher_Encrypt(b *testing.B) {
    ks := encryption.NewKeystore()
    dek := make([]byte, encryption.KeySize)
    _ = ks.Set(1, dek)
    c, _ := encryption.NewCipher(ks)
    plain := bytes.Repeat([]byte("x"), 4096)
    nonce := make([]byte, encryption.NonceSize)
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        _, _ = c.Encrypt(plain, nil, 1, nonce)
    }
}

Fix this →

---

Minor

4. kek/file.go:76 — nil AAD in Wrap is still unexplained

out = w.aead.Seal(out, nonce, dek, nil)

Intentional but undocumented. A future maintainer adding key-ID binding at the KEK layer might add AAD here in isolation and silently break the format for existing persisted blobs. One line suffices:

// AAD is nil: key-ID binding at the KEK layer is deferred to Stage 9.
out = w.aead.Seal(out, nonce, dek, nil)

Fix this →

---

5. cipher_test.go:14 — stale comment references removed Get method

// newKeystoreWithKey returns a Keystore with one freshly-drawn DEK at
// testKeyID. Tests that need the raw DEK bytes can call ks.Get(testKeyID).

The Get method no longer exists — the API is now DEK(keyID) returning ([KeySize]byte, bool). Update to:

// Tests that need the raw DEK bytes can call ks.DEK(testKeyID).

Fix this →

---

Confirmed Fixed Since Round 3

• NewCipher(nil) returns ErrNilKeystore — cipher.go:31-35 correctly returns (*Cipher, error); TestNewCipher_RejectsNil locks it down. The choice to return a typed error rather than panic is well-reasoned (dynamic wiring paths noted at cipher.go:28-30). ✅
• ErrNilKeystore added to errors.go:38-41 with clear doc comment. ✅
• mustCipher helper updated to handle the new error signature. ✅

---

Reviewer Checklist

• Cipher API does not bake storage/raft AAD — Confirmed. Encrypt/Decrypt accept raw aad []byte; §4.1 and §4.2 composition is left entirely to callers. ✅
• ReservedKeyID = 0 rejected at all entry points — Confirmed via aeadFor for both Encrypt and Decrypt, and Set guard in Keystore. All directions are tested. ✅

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 2m 25s —— View job

---

Code Review: feat/encryption-foundation (Stage 0) — Round 5

• Gather context from prior reviews (4 rounds)
• Read current state of all changed files
• Check which prior round items remain open
• Review for new issues
• Post findings

---

Excellent progress since Round 4. All four open items from that round have been addressed. The PR is in very good shape — only one stale comment carried forward remains, plus one new minor style issue in the benchmark helper.

---

Confirmed Fixed Since Round 4

• Review-provenance parentheticals removed — keystore.go:12-19 now has the clean rationale comment with no "(gemini-code-assist review on PR feat(encryption): foundation package (Stage 0) #719)" or "(claude[bot] review)" labels. ✅
• Zero-value safety warning added — keystore.go:34-37 now clearly documents var ks Keystore is not safe and directs callers to NewKeystore. ✅
• Benchmarks added — cipher_bench_test.go provides BenchmarkCipher_Encrypt, BenchmarkCipher_Decrypt, and BenchmarkKeystore_AEAD across four payload sizes. The b.SetBytes + b.ReportAllocs() pattern is correct. ✅
• nil AAD explained in FileWrapper.Wrap — kek/file.go:76-81 now has a five-line comment explaining the deferral, why doing it now would break persisted blobs, and when it lands (Stage 9). More thorough than the minimal one-liner requested. ✅

---

Minor (two items remaining)

1. Stale comment in cipher_test.go:15 — carried from Round 4

// newKeystoreWithKey returns a Keystore with one freshly-drawn DEK at
// testKeyID. Tests that need the raw DEK bytes can call ks.Get(testKeyID).

The Get method no longer exists; the API is now DEK(keyID) returning ([KeySize]byte, bool). Change to:

// Tests that need the raw DEK bytes can call ks.DEK(testKeyID).

Fix this →

---

2. formatBytes in cipher_bench_test.go:127-138 — O(n²) prepend loop

for n > 0 {
    digits = append([]byte{byte('0' + n%10)}, digits...)
    n /= 10
}

append([]byte{d}, digits...) allocates and copies on every iteration — O(k²) in the digit count. For the benchmark sizes (≤5 digits) this is inconsequential, but the comment at line 128-129 justifying it ("avoid fmt.Sprintf to keep the benchmark file self-contained") doesn't hold: strconv is already in stdlib with no transitive imports and strconv.Itoa(n) + unit is one line:

import "strconv"

func name(size int) string {
    switch {
    case size < 1024:
        return strconv.Itoa(size) + "B"
    case size < 1024*1024:
        return strconv.Itoa(size/1024) + "KiB"
    default:
        return strconv.Itoa(size/(1024*1024)) + "MiB"
    }
}

The entire formatBytes helper can then be deleted.

Fix this →

---

Reviewer Checklist

• Cipher API does not bake storage/raft AAD — Confirmed. Encrypt/Decrypt accept raw aad []byte; §4.1 and §4.2 composition is entirely the caller's responsibility. ✅
• ReservedKeyID = 0 rejected at all entry points — Confirmed in code and test coverage: aeadFor gates both Encrypt and Decrypt, Keystore.Set guards it, and TestCipher_Encrypt_RejectsReservedKeyID + TestCipher_Decrypt_RejectsReservedKeyID + TestKeystore_Set_RejectsReservedKeyID cover all directions. ✅

---

The two items above are cosmetic. The package is functionally correct, the keystore redesign is solid, error propagation follows project conventions, and the test/benchmark coverage is thorough. This is ready to merge once item 1 is patched.

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 1m 9s —— View job

---

Code Review: feat/encryption-foundation (Stage 0) — Round 6

• Gather context from prior reviews (5 rounds)
• Read current state of all changed files
• Check which Round 5 items are resolved
• Review for new issues
• Post findings

---

Both Round 5 items have been checked. One has landed, one remains.

---

Confirmed Fixed Since Round 5

• Stale comment in cipher_test.go:15 — "ks.Get(testKeyID)" is now correctly "ks.DEK(testKeyID)". ✅

---

Minor (one item remaining)

formatBytes in cipher_bench_test.go:127-138 — O(n²) prepend loop

This was Round 5 item 2. The loop still reads:

for n > 0 {
    digits = append([]byte{byte('0' + n%10)}, digits...)
    n /= 10
}

append([]byte{d}, digits...) allocates and copies on every iteration. For 5-digit numbers that's 5 allocations. The comment justifying the approach says "avoid fmt.Sprintf to keep the benchmark file self-contained", but strconv is already in the Go stdlib with zero transitive imports. The entire helper can be collapsed to:

import "strconv"

func name(size int) string {
    switch {
    case size < 1024:
        return strconv.Itoa(size) + "B"
    case size < 1024*1024:
        return strconv.Itoa(size/1024) + "KiB"
    default:
        return strconv.Itoa(size/(1024*1024)) + "MiB"
    }
}

Delete formatBytes entirely and inline strconv.Itoa into name. One import, zero helper.

Fix this →

---

Reviewer Checklist

• Cipher API does not bake storage/raft AAD — Confirmed. Encrypt/Decrypt accept raw aad []byte; §4.1 and §4.2 composition is entirely the caller's responsibility. ✅
• ReservedKeyID = 0 rejected at all entry points — Confirmed in code (aeadFor gates both Encrypt and Decrypt, Keystore.Set guards it) and in tests (TestCipher_Encrypt_RejectsReservedKeyID, TestCipher_Decrypt_RejectsReservedKeyID, TestKeystore_Set_RejectsReservedKeyID). ✅

---

The formatBytes item is cosmetic. The package is functionally correct, the architecture is sound, error propagation follows project conventions, and the test/benchmark coverage is thorough. This is ready to merge.

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 2m 34s —— View job

---

Code Review: feat/encryption-foundation (Stage 0) — Round 7

• Gather context from prior reviews (6 rounds)
• Read current state of all changed files
• Check which Round 6 items are resolved
• Review for new issues
• Post findings

---

Confirmed Fixed Since Round 6

The only open item from Round 6 was the formatBytes O(n²) prepend loop in cipher_bench_test.go. Commit ce23dcd resolved this: formatBytes is gone entirely and the name helper now uses strconv.Itoa directly — clean one-liner per size tier, zero unnecessary allocations. ✅

---

New Observation (very minor)

Encrypt doc is slightly misleading about crypto/rand nonces

cipher.go:50-52:

// The §4.1 nonce construction
// (node_id ‖ local_epoch ‖ write_count) is designed to make reuse
// impossible by construction; do not bypass it with crypto/rand or
// external counters without re-deriving the same uniqueness proof.

The phrase "do not bypass it with crypto/rand" reads as advice against using crypto/rand for nonces — but crypto/rand is perfectly safe for nonce generation (it's what the benchmarks and tests use). What the sentence actually means is: if you're wiring the §4.1 integration pipeline, keep using the §4.1 nonce construction rather than substituting a different scheme without proving equivalent uniqueness. Not a bug, but a future integration developer reading this cold could reasonably conclude they must not use crypto/rand.

Suggest rephrasing to:

// The §4.1 storage-layer integration uses the nonce construction
// (node_id ‖ local_epoch ‖ write_count) to guarantee uniqueness by
// construction. Do not substitute a different nonce scheme in that
// layer without a corresponding uniqueness proof.

This is not a blocker — the surrounding context makes the intent clear enough. Noting for completeness.

---

Full Confirmed Status (all rounds)

Every item raised across Rounds 1–6 has been addressed:

Round	Item	Status
R1	Nonce-reuse warning in Encrypt doc	✅
R1	Keystore.Get mutable slice → DEK() [KeySize]byte value copy	✅
R1	TestCipher_Decrypt_RejectsReservedKeyID / RejectsBadNonceSize	✅
R1	EnvelopeVersionV1 comment clarified	✅
R1	Encode() validates and returns ([]byte, error)	✅
R1	sort.Slice → slices.Sort	✅
R1	FileWrapper.Name() returns "file:"+path	✅
R1	Unreachable post-Open length check removed	✅
R1	nil AAD in Wrap documented (Stage 9 deferral)	✅
R3	Review-provenance parentheticals removed	✅
R3	Zero-value safety warning on Keystore type	✅
R3	NewCipher(nil) returns ErrNilKeystore	✅
R3	Benchmarks added (Encrypt, Decrypt, Keystore_AEAD)	✅
R4	Stale ks.Get(testKeyID) → ks.DEK(testKeyID) comment	✅
R5/R6	formatBytes O(n²) loop → strconv.Itoa	✅

---

Reviewer Checklist

• Cipher API does not bake storage/raft AAD — Confirmed. Encrypt/Decrypt accept raw aad []byte; §4.1 and §4.2 AAD composition is entirely the caller's responsibility. cipher.go:9-14 documents this explicitly. ✅
• ReservedKeyID = 0 rejected at all entry points — Confirmed in code (aeadFor gates both Encrypt and Decrypt at cipher.go:96, Keystore.Set at keystore.go:94) and in tests (TestCipher_Encrypt_RejectsReservedKeyID, TestCipher_Decrypt_RejectsReservedKeyID, TestKeystore_Set_RejectsReservedKeyID). ✅

---

The package is correct, the architecture is sound, the keystore redesign (pre-initialized keyEntry.aead, [KeySize]byte immutability) is the right long-term shape, and the test and benchmark coverage is thorough. Ready to merge.

[bootjp]

@claude review

[claude]

Claude finished @bootjp's task in 2m 18s —— View job

---

Code Review: feat/encryption-foundation (Stage 0) — Round 8

• Gather context from prior reviews (7 rounds)
• Read current state of all changed files
• Check which Round 7 items are resolved
• Review for new issues
• Post findings

---

Confirmed Fixed Since Round 7

The only new item raised in Round 7 was the slightly misleading nonce-uniqueness doc comment. Commit c3f41a4 addressed it cleanly — cipher.go:53 now explicitly adds:

(For tests / benchmarks, fresh crypto/rand nonces are perfectly safe.)

This resolves the forward-compatibility ambiguity without watering down the critical nonce-reuse warning. ✅

---

No New Issues Found

Full pass across all 13 changed files. Nothing to flag.

---

Full Confirmed Status (all 8 rounds)

Round	Item	Status
R1	Nonce-reuse warning in Encrypt doc	✅
R1	Keystore.Get mutable slice → DEK() [KeySize]byte value copy	✅
R1	TestCipher_Decrypt_RejectsReservedKeyID / RejectsBadNonceSize	✅
R1	EnvelopeVersionV1 comment clarified	✅
R1	Encode() validates and returns ([]byte, error)	✅
R1	sort.Slice → slices.Sort	✅
R1	FileWrapper.Name() returns "file:"+path	✅
R1	Unreachable post-Open length check removed	✅
R1	nil AAD in Wrap documented (Stage 9 deferral)	✅
R3	Review-provenance parentheticals removed from keystore.go	✅
R3	Zero-value safety warning on Keystore type	✅
R3	NewCipher(nil) returns ErrNilKeystore	✅
R3	Benchmarks added (Encrypt, Decrypt, Keystore_AEAD)	✅
R4	Stale ks.Get(testKeyID) → ks.DEK(testKeyID) comment	✅
R5/R6	formatBytes O(n²) loop → strconv.Itoa	✅
R7	Nonce doc clarified: crypto/rand is safe for tests/benchmarks	✅

---

Reviewer Checklist

• Cipher API does not bake storage/raft AAD — Confirmed. Encrypt/Decrypt accept raw aad []byte; cipher.go:9-14 documents the §4.1 / §4.2 orthogonality contract explicitly. ✅
• ReservedKeyID = 0 rejected at all entry points — Confirmed in code (aeadFor gates both Encrypt and Decrypt at cipher.go:98, Keystore.Set at keystore.go:94) and in tests (TestCipher_Encrypt_RejectsReservedKeyID, TestCipher_Decrypt_RejectsReservedKeyID, TestKeystore_Set_RejectsReservedKeyID). ✅

---

The package is functionally correct, the architecture is sound, the keystore redesign (pre-initialized keyEntry.aead, [KeySize]byte immutability, copy-on-write via atomic.Pointer) is the right long-term shape, and the test + benchmark coverage is thorough. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3f41a4d60

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Reject uninitialized Cipher before using keystore

Cipher is exported, so callers can create a zero-value instance (var c encryption.Cipher) without going through NewCipher; in that case c.keystore is nil and Encrypt/Decrypt panic when aeadFor calls c.keystore.AEAD. This turns a construction mistake into a runtime crash in any package that uses this internal API directly, so aeadFor should defensively return a typed error when c or c.keystore is nil.

Useful? React with 👍 / 👎.