review: cover derive_key with the official vectors, fix a backwards timing doc

Addresses the CodeRabbit pass on #268. Taking the findings that hold and
saying which do not, rather than applying all nine.

FIXED — correctness:

- **`derive_key` had zero vector coverage.** The parser read `input_len`,
  `hash`, and `keyed_hash` and skipped past the `derive_key` field already
  present in the file. It is the one public mode built as TWO passes
  (DERIVE_KEY_CONTEXT over the context string, whose output keys a
  DERIVE_KEY_MATERIAL pass over the material), so a swapped or collapsed
  transcription would have been invisible while hash/keyed_hash stayed green.
  Now asserted for all 35 cases at both the 32-byte and extended XOF lengths.
  It passed on the first run — the transcription was right; it just wasn't
  proven. Control-tested: swapping the two flags fails ONLY the new assertion.

- **`Hash::eq`'s documentation described the opposite of the code.** The
  doc-comment said `read_volatile` (the code uses `core::hint::black_box`) and
  the not-transcribed list flatly asserted "`PartialEq` here is a normal
  short-circuiting byte-array compare" — stale since the constant-time fold
  landed. On a MAC-comparison path a backwards timing claim is worse than no
  claim: an auditor would have believed this file leaks match-prefix length
  when it does not. Also now states that `black_box` is a best-effort barrier,
  not a guarantee; the data-independent loop is the actual property.

FIXED — hazards:

- `run.sh` copied the bench to a fixed `examples/blake3_ab.rs` and had the EXIT
  trap `rm` it unconditionally: a pre-existing file there would be clobbered
  and then deleted. Now refuses to overwrite, and only removes the directory it
  created.
- `shiftor_rot_u64x8` is `pub` and took arbitrary `u32` into `a >> n` — a debug
  panic and a release wrapping-shift for `n >= 64`. Normalized at the PUBLIC
  wrapper and deliberately NOT in the measured inner body, which would have
  changed the codegen the oracle exists to observe.
- Rotate test extended past 64 (65/127/128/191) against std's `u64::rotate_*`,
  pinning the mod-64 contract rather than only the width case.
- Test-local context const renamed `VECTORS_CONTEXT`; as `DERIVE_KEY_CONTEXT`
  it shadowed the module-scope u32 flag of that name.
- Redundant `.into()` in `derive_key` (`clippy::useless_conversion`).
- The `pub mod blake3` incident history was an OUTER doc comment, so rustdoc
  would publish it as module documentation. Now a plain `//` comment.

Three more stale doc claims, all the same class already corrected elsewhere in
this PR — an assertion outliving the change that falsified it:

- EPIPHANIES said "an error there means no cycle and the work is unblocked"
  one paragraph after explaining the error is ambiguous. Read literally it
  marks blocked work unblocked on the strength of a typo. Now a four-step
  procedure with the error explicitly inconclusive.
- crypto-lane-status prescribed an explicit shift-or for AVX2/NEON/wasm,
  contradicting the measurement directly above it (LLVM folds the two forms
  byte-identically) and misdescribing what shipped.
- blake3-in-tree-measured still credited the swap with removing the C build,
  already removed by `features = ["pure"]` in #264.

NOT taken, with reasons:

- Doctest examples on every public API. Worth doing, but it is a separate
  change across a surface this PR is not otherwise touching, and the vectors
  already smoke-test the public path harder than an example would.
- Caching the compressed block in `OutputReader`. A real inefficiency for
  small repeated `fill` calls, but unmeasured, and the module is explicitly
  not on a hot path pending the swap ruling.
- Deduplicating the rotate body across backends. Every other lane type in this
  crate is duplicated per backend by design; collapsing one method would make
  it the exception, not the rule.
- Secret-scanner allowlist for the vectors file. Correct that the hex is
  published test data, not credentials, but no scanner config lives in this
  repo to add it to.

Full lib suite green: 2200 passed, 0 failed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfbkUCBbtZhy3yjSfCDHp

Author: claude

.claude/board/EPIPHANIES.md

@@ -65,14 +65,23 @@ caught it on #268. Treating `[[patch.unused]]` as a cycle report teaches the
 next reader to misdiagnose an ordinary stale patch — and undermines the very
 check this entry prescribes.
 
-The check stands, on its own evidence: run
-`cargo tree -p <our-root> -i <X>` **before** planning the work. An error
-there means no cycle and the work is unblocked; a hit means the edge must be
-cut first.
+The check stands, but ONLY in its positive form — and the wording here was
+itself an instance of the bug it warns about, corrected on #268:
 
 Consequence: **before planning any "make X consume our crate" work, run
-`cargo tree -p <our-root> -i <X>`.** An error there means no cycle and the
-work is unblocked; a hit means the edge must be cut first.
+`cargo tree -p <our-root> -i <X>`.**
+
+1. First confirm `<X>` is a real package in the selected graph (a typo, or a
+   package absent from that graph, produces the byte-identical error).
+2. A tree that resolves and shows the root package = the edge must be cut
+   first.
+3. A tree that resolves and does NOT show it = unblocked.
+4. **A package-ID error is inconclusive — never "no cycle".**
+
+An earlier draft of this very entry said "an error there means no cycle and
+the work is unblocked", one paragraph after explaining that the error is
+ambiguous. Read literally it would mark blocked work as unblocked on the
+strength of a typo.
 
 
 ## 2026-07-29 — BLAKE3 needs a method surface, not intrinsics (measured)

.claude/knowledge/blake3-ab-bench/run.sh

@@ -10,8 +10,23 @@
 set -eu
 HERE="$(cd "$(dirname "$0")" && pwd)"
 REPO="$(cd "$HERE/../../.." && pwd)"
-mkdir -p "$REPO/examples"
-trap 'rm -f "$REPO/examples/blake3_ab.rs"; rmdir "$REPO/examples" 2>/dev/null || true' EXIT
-cp "$HERE/blake3_ab.rs" "$REPO/examples/blake3_ab.rs"
+# The bench needs its source under $REPO/examples/ for cargo to see it, and
+# removes it afterwards. Both halves must refuse to touch anything they did not
+# create: a fixed destination plus an unconditional `rm` in the EXIT trap would
+# clobber a pre-existing examples/blake3_ab.rs and then delete it.
+EXAMPLE_DIR="$REPO/examples"
+EXAMPLE="$EXAMPLE_DIR/blake3_ab.rs"
+created_example_dir=0
+if [ -e "$EXAMPLE" ] || [ -L "$EXAMPLE" ]; then
+    echo "refusing to overwrite $EXAMPLE" >&2
+    exit 1
+fi
+if [ ! -d "$EXAMPLE_DIR" ]; then
+    mkdir "$EXAMPLE_DIR"
+    created_example_dir=1
+fi
+# Only remove the directory if this script created it.
+trap 'rm -f "$EXAMPLE"; [ "$created_example_dir" -eq 0 ] || rmdir "$EXAMPLE_DIR" 2>/dev/null || true' EXIT
+cp "$HERE/blake3_ab.rs" "$EXAMPLE"
 cd "$REPO"
 cargo run --release --quiet --example blake3_ab

.claude/knowledge/blake3-in-tree-measured.md

@@ -114,8 +114,15 @@ So the honest shape is:
 
 ## What this means for the swap
 
-**The cycle-cut is available now and is correct.** It removes a cargo cycle,
-the C build question, and 2,910 lines of second-surface `core::arch`.
+**The cycle-cut is available now and is correct.** It removes a cargo cycle
+and 2,910 lines of second-surface `core::arch`.
+
+It does **not** remove a C build — `Cargo.toml:213` already sets
+`default-features = false, features = ["pure"]`, which removed all C/ASM
+compilation back in #264. An earlier revision of this line credited the swap
+with that too; overstating the benefit matters here specifically, because
+what it is being weighed against is transcribing a cryptographic
+implementation.
 
 **It is not free**, and the previous framing ("removes things, needs no
 benchmark to justify") was true about what it *removes* and silent about what

.claude/knowledge/crypto-lane-status.md

@@ -116,9 +116,21 @@ failing row, since a probe the compiler cannot distinguish from its own
 control measures nothing.
 
 **So the u64 ARX lane is the crate's first intrinsic override that meets the
-entry criterion** (a probe proving the generic form fails). AVX-512:
-`_mm512_rorv_epi64` / `VPROLVQ`, one instruction. AVX2 / NEON / wasm: write
-the `vpsllq`/`vpsrlq`-shaped shift-or explicitly, since LLVM will not.
+entry criterion** (a probe proving the generic form fails).
+
+What SHIPPED, per backend (#268), which is not uniform:
+
+- **AVX-512:** `_mm512_rolv_epi64` / `_mm512_rorv_epi64` — `VPROLVQ`/`VPRORVQ`,
+  one instruction. This is the earned override.
+- **AVX2 / scalar / nightly:** per-lane loops. NEON and wasm re-export the
+  scalar `U64x8` and are covered by that arm.
+
+An earlier draft of this paragraph instructed AVX2/NEON/wasm to "write the
+`vpsllq`/`vpsrlq`-shaped shift-or explicitly, since LLVM will not." That
+prescription contradicted the fourth confirmation directly above it — LLVM
+folded the explicit shift-or into the rotate form, byte-identically, so
+writing it out is not known to change anything. **It is an unmeasured future
+experiment, not the prescribed implementation**, and it is not what shipped.
 
 Contrast with the u32 lane, where hand-writing intrinsics *lost* to the
 optimizer. Same crate, same week, opposite answers — which is the argument

.claude/knowledge/simd-codegen-oracle/probes.rs

@@ -520,8 +520,22 @@ fn shiftor_rotr_u64x8(v: U64x8, n: u32) -> U64x8 {
 }
 
 /// Explicit shift-or u64 rotate, runtime-variable amount, 8 lanes.
+///
+/// Normalizes `n` to `0..64` and short-circuits zero, so this public entry
+/// point agrees with `u64::rotate_right` at **every** `u32` input rather than
+/// only on `1..=63`. Without it, `n >= 64` reaches `a >> n` — a panic in
+/// debug, a wrapping shift in release, and a rotate in neither.
+///
+/// The normalization lives HERE and deliberately NOT in `shiftor_rotr_u64x8`:
+/// that inner body is the thing being measured, and adding a branch plus a
+/// modulo inside it would change the very codegen the oracle exists to
+/// observe. Guard at the boundary; measure the bare form.
 #[inline(never)]
 pub fn shiftor_rot_u64x8(v: U64x8, n: u32) -> U64x8 {
+    let n = n % 64;
+    if n == 0 {
+        return v;
+    }
     shiftor_rotr_u64x8(v, n)
 }
 

src/hpc/blake3.rs

@@ -375,8 +375,14 @@ impl PartialEq for Hash {
     /// `==` leaks match-prefix length through timing. Upstream reaches for
     /// the `constant_time_eq` crate; this crate takes no new dependencies, so
     /// the fold is written here: accumulate the XOR of every byte pair and
-    /// test once at the end, with `read_volatile` on the accumulator to stop
-    /// the optimizer reintroducing an early exit.
+    /// test once at the end, with `core::hint::black_box` on the accumulator
+    /// to stop the optimizer reintroducing an early exit.
+    ///
+    /// `black_box` is a **best-effort optimizer barrier, not a guarantee** —
+    /// its documentation is explicit that it provides no formal contract. The
+    /// data-independent loop above is the real property; the barrier only
+    /// discourages LLVM from undoing it. A caller needing an audited
+    /// guarantee should use a dedicated constant-time crate.
     ///
     /// No call site in this crate currently compares two `Hash` values —
     /// `seal.rs` compares the truncated `MerkleRoot` instead — so this is a
@@ -387,8 +393,8 @@ impl PartialEq for Hash {
         for i in 0..OUT_LEN {
             diff |= self.0[i] ^ other.0[i];
         }
-        // SAFETY-free volatile read of a local: prevents the compiler from
-        // proving an early return is equivalent.
+        // Optimizer barrier (best-effort): discourages LLVM from proving an
+        // early return equivalent and reintroducing the short circuit.
         core::hint::black_box(diff) == 0
     }
 }
@@ -608,7 +614,7 @@ pub fn keyed_hash(key: &[u8; KEY_LEN], input: &[u8]) -> Hash {
 pub fn derive_key(context: &str, key_material: &[u8]) -> [u8; OUT_LEN] {
     let mut hasher = Hasher::new_derive_key(context);
     hasher.update(key_material);
-    (*hasher.finalize().as_bytes()).into()
+    *hasher.finalize().as_bytes()
 }
 
 // =======================================================================
@@ -624,13 +630,16 @@ pub fn derive_key(context: &str, key_material: &[u8]) -> [u8; OUT_LEN] {
 // - `update_rayon` and anything behind the `rayon` feature.
 // - `zeroize` support (feature-gated in the original; secrets are not a
 //   concern for ndarray's usage of this module).
-// - Constant-time equality (`constant_time_eq`): the original `Hash::eq` is
-//   constant-time as a defense-in-depth measure for MAC comparisons. Adding
-//   that would mean hand-rolling a constant-time byte compare with no
-//   external crate; out of scope for what ndarray needs (a plain fingerprint
-//   equality check), so `PartialEq` here is a normal short-circuiting
-//   byte-array compare. Flagged explicitly since it is a behavioral
-//   difference from upstream `blake3::Hash`, not just a missing convenience.
+// - The `constant_time_eq` CRATE — the dependency, NOT the behaviour. The
+//   constant-time compare itself IS implemented: see `impl PartialEq for
+//   Hash`, which XOR-folds all 32 bytes and tests once, so `Hash::eq` keeps
+//   upstream's timing property without adding the dependency.
+//
+//   An earlier revision of this list claimed the opposite — "`PartialEq` here
+//   is a normal short-circuiting byte-array compare" — left stale when the
+//   fold landed. Documenting a timing property backwards is worse than
+//   omitting it: someone auditing a MAC comparison would have believed this
+//   file leaks match-prefix length when it does not.
 // - Hex encoding/decoding (`to_hex`/`from_hex`) and `Display`/`FromStr`: not
 //   in the required API list.
 // =======================================================================
@@ -648,8 +657,17 @@ mod tests {
         input_len: usize,
         hash_hex: String,
         keyed_hash_hex: String,
+        derive_key_hex: String,
     }
 
+    /// The context string the official vectors' `derive_key` outputs were
+    /// generated with (the `context_string` field of `test_vectors.json`).
+    ///
+    /// Deliberately NOT named `DERIVE_KEY_CONTEXT` — that is a `u32` domain
+    /// flag at module scope, and reusing the name here would shadow it inside
+    /// this module.
+    const VECTORS_CONTEXT: &str = "BLAKE3 2019-12-27 16:29:52 test vectors context";
+
     /// Hand-rolled extraction of the fields we need from the official BLAKE3
     /// test_vectors.json, without pulling in serde. The file's `cases` array
     /// is a flat sequence of objects each with exactly the fields
@@ -683,10 +701,19 @@ mod tests {
             let keyed_hash_hex = extract_quoted(rest);
             rest = &rest[keyed_hash_hex.len() + 2..];
 
+            let dk_idx = rest
+                .find("\"derive_key\":")
+                .expect("missing derive_key field")
+                + "\"derive_key\":".len();
+            rest = &rest[dk_idx..];
+            let derive_key_hex = extract_quoted(rest);
+            rest = &rest[derive_key_hex.len() + 2..];
+
             cases.push(Case {
                 input_len,
                 hash_hex,
                 keyed_hash_hex,
+                derive_key_hex,
             });
         }
         cases
@@ -717,7 +744,7 @@ mod tests {
     }
 
     #[test]
-    fn official_test_vectors_hash_and_keyed_hash() {
+    fn official_test_vectors_all_three_modes() {
         let cases = parse_cases(VECTORS_JSON);
         assert!(!cases.is_empty(), "no cases parsed");
         let mut checked = 0usize;
@@ -763,6 +790,31 @@ mod tests {
                 case.input_len
             );
 
+            // --- derive_key, 32-byte default output ---
+            //
+            // The third public mode, and the one whose flags are easiest to
+            // get wrong while the other two still pass: it is a TWO-pass
+            // construction (DERIVE_KEY_CONTEXT over the context string,
+            // whose output becomes the key words for a DERIVE_KEY_MATERIAL
+            // pass over the material). A single-pass transcription, or one
+            // that swapped the two flags, would be invisible to every
+            // assertion above.
+            let expected_dk = hex_decode(&case.derive_key_hex);
+            let got_dk = derive_key(VECTORS_CONTEXT, &input);
+            assert_eq!(got_dk[..], expected_dk[..32], "derive_key() mismatch at input_len={}", case.input_len);
+
+            // --- derive_key, extended output via finalize_xof ---
+            let mut dhasher = Hasher::new_derive_key(VECTORS_CONTEXT);
+            dhasher.update(&input);
+            let mut dxof = dhasher.finalize_xof();
+            let mut dextended = std::vec![0u8; expected_dk.len()];
+            dxof.fill(&mut dextended);
+            assert_eq!(
+                dextended, expected_dk,
+                "derive_key finalize_xof extended output mismatch at input_len={}",
+                case.input_len
+            );
+
             checked += 1;
         }
         // Sanity: make sure we actually walked the whole official vector file.

src/hpc/mod.rs

@@ -54,13 +54,17 @@ pub mod fingerprint;
 #[allow(missing_docs)]
 pub mod plane;
 
-/// In-tree BLAKE3 — see `.claude/knowledge/blake3-in-tree-measured.md`.
-///
-/// Fully documented, so deliberately NOT under the `#[allow(missing_docs)]`
-/// that guards `plane`. An earlier revision declared it on the line directly
-/// after that attribute, which silently re-targeted the attribute onto this
-/// module and left `plane` unguarded — seven CI jobs failed on `missing_docs`
-/// errors in a file the change never touched.
+// In-tree BLAKE3 — see `.claude/knowledge/blake3-in-tree-measured.md`.
+//
+// A plain `//` comment, not `///`: this is repo incident history, and an outer
+// doc comment here would be concatenated into the published rustdoc for the
+// module.
+//
+// The module is fully documented, so it is deliberately NOT under the
+// `#[allow(missing_docs)]` that guards `plane`. An earlier revision declared
+// it on the line directly after that attribute, which silently re-targeted the
+// attribute onto this module and left `plane` unguarded — seven CI jobs failed
+// on `missing_docs` errors in a file the change never touched.
 pub mod blake3;
 #[allow(missing_docs)]
 pub mod seal;

src/simd.rs

@@ -768,7 +768,12 @@ mod tests {
 
         // BLAKE2b uses 32/24/16/63; 0 and 63 are the edges, and 64 must wrap
         // to the identity rather than shifting by the full width (UB on u64).
-        for n in [0u32, 1, 7, 16, 24, 31, 32, 63, 64] {
+        // 65/127/128/191 pin the documented mod-64 contract: an implementation
+        // that only special-cased the width would pass at 64 and fail here.
+        // The per-lane oracle is std's own `u64::rotate_left`, which is
+        // specified to take its count mod 64, so this asserts agreement with
+        // the language rather than with our own restatement of the rule.
+        for n in [0u32, 1, 7, 16, 24, 31, 32, 63, 64, 65, 127, 128, 191] {
             let l = a.rotate_left(n).to_array();
             let r = a.rotate_right(n).to_array();
             for i in 0..8 {