fix(rust): deterministic clustered vcov — first-appearance cluster aggregation

[igerber]

Summary

• Resolves the run-to-run nondeterminism discovered this session: Rust compute_robust_vcov (also reached via solve_ols(return_vcov=True)) built its (G, k) cluster-scores matrix in HashMap iteration order — SipHash-randomized per call. Mathematically identical, but the GEMM accumulation order changed on every invocation, wobbling the clustered vcov at ~1e-14 (3 distinct values across 8 identical calls; the Python backend is bit-stable; thread pinning had no effect, which pointed away from BLAS and at the map).
• Rows now accumulate in first-appearance order — ascending for the factorized 0..G-1 ids the Python dispatcher passes, matching NumPy's groupby order. Side benefit: the per-row to_owned + map-entry churn is gone (direct accumulation into the matrix).
• Verified: 1 distinct value across 50 identical calls (was 3-in-8); NumPy parity unchanged (~1e-14, the normal cross-backend GEMM tolerance per the repo's assert_allclose convention). Bit-identity regression tests cover contiguous ids (the dispatcher path) and non-contiguous unsorted ids (the direct-kernel path exercising the remap).
• The TODO row documenting this finding is currently in flight on perf(utils): precompute r-independent wild-bootstrap inversion pieces (~7x, outputs unchanged) #647; its deletion follows in the next TODO-touching PR once both merge (noted here to avoid a cross-PR conflict).

Methodology references (required if estimator / math changes)

• Method name(s): CR1 cluster-robust sandwich (no formula change — summation-order determinism only; Python numpy implementation is canonical per repo convention)
• Paper / source link(s): N/A — numerical implementation detail
• Any intentional deviations from the source (and why): None

Validation

• Tests added/updated: tests/test_rust_backend.py::TestClusterVcovDeterminism (2 bit-identity tests, 20 repeat-calls each).
• Evidence: 50-call probe → 1 unique value; tests/test_rust_backend.py + tests/test_linalg.py pass (253); cargo fmt/check clean.

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected method: CR1 cluster-robust sandwich vcov in Rust compute_robust_vcov_internal, reached by compute_robust_vcov and solve_ols(return_vcov=True).
• Methodology matches the registry/docstring contract: cluster meat remains sum_g score_g score_g' and CR1 adjustment remains (G/(G-1))*((N-1)/(N-k)).
• The change is a deterministic accumulation-order implementation detail, not a formula/default/assumption change.
• No NaN/Inf inference anti-patterns, control-group changes, parameter propagation issues, or security concerns found.
• One P3 test/documentation note: the new public-wrapper test factorizes “non-contiguous” IDs before Rust, so it does not directly test raw Rust non-contiguous IDs.

Methodology

No blocking findings.

Severity: N/A
Impact: The Rust code still computes the same CR1 cluster-robust sandwich specified in docs/methodology/REGISTRY.md:L126-L128 and diff_diff/linalg.py:L1693-L1696. The only changed behavior is deterministic cluster-row ordering before cluster_scores.t().dot(&cluster_scores) at rust/src/linalg.rs:L284-L318.
Concrete fix: None required.

Code Quality

No findings.

Severity: N/A
Impact: The new cluster_index map is only used for lookup, not iteration, so randomized HashMap ordering no longer affects vcov row order.
Concrete fix: None required.

Performance

No findings.

Severity: N/A
Impact: The change removes per-row to_owned() accumulation into map values and uses direct matrix row accumulation at rust/src/linalg.rs:L309-L314.
Concrete fix: None required.

Maintainability

No findings.

Severity: N/A
Impact: The Rust comment explains why deterministic ordering matters and preserves the CR1 formula context.
Concrete fix: None required.

Tech Debt

No findings.

Severity: N/A
Impact: No new deferred correctness limitation introduced.
Concrete fix: None required.

Security

No findings.

Severity: N/A
Impact: No secrets, unsafe code, or input-handling security concern introduced by the diff.
Concrete fix: None required.

Documentation/Tests

Severity: P3
Impact: The added test tests/test_rust_backend.py:L3535-L3545 imports diff_diff.linalg.compute_robust_vcov, whose wrapper factorizes cluster_ids before calling Rust at diff_diff/linalg.py:L1722-L1727. So the test covers public API non-contiguous IDs, but not raw _rust_backend.compute_robust_vcov receiving non-contiguous unsorted IDs directly.
Concrete fix: Optional: add a small raw Rust-backend regression importing diff_diff._rust_backend.compute_robust_vcov with the same [7, 3, 11, ...] cluster IDs and asserting repeated-call bit identity.

Tests not run in this read-only review sandbox.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 2f76a321bf0c0babfa73ae28e8c1839101f09645

---

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected method: Rust CR1 clustered sandwich vcov in compute_robust_vcov_internal, reached by _rust_backend.compute_robust_vcov and solve_ols(return_vcov=True).
• Methodology matches the registry/docstring contract: cluster meat remains Σ_g (X_g'u_g)(X_g'u_g)' with CR1 correction (G/(G-1))*((N-1)/(N-k)).
• The PR changes deterministic row aggregation order only; no estimator formula, default, identification assumption, inference field, or SE formula changes were found.
• The prior P3 test note remains: the public-wrapper “non-contiguous” test factorizes IDs before Rust, so it does not directly test raw Rust non-contiguous IDs.
• I could not run tests in this sandbox because numpy is unavailable.

Methodology

No blocking findings.

Severity: N/A
Impact: The changed Rust path still implements the CR1 contract documented in docs/methodology/REGISTRY.md:L126-L128 and diff_diff/linalg.py:L1694-L1696. The meat and adjustment at rust/src/linalg.rs:L385-L391 are unchanged in substance.
Concrete fix: None required.

Code Quality

No findings.

Severity: N/A
Impact: cluster_index is used only for deterministic lookup, not iteration, so HashMap randomized iteration order no longer affects the cluster-score row order at rust/src/linalg.rs:L361-L383.
Concrete fix: None required.

Performance

No findings.

Severity: N/A
Impact: The change removes per-observation to_owned() row allocation and direct map-value array replacement, replacing it with direct accumulation into cluster_scores.
Concrete fix: None required.

Maintainability

No findings.

Severity: N/A
Impact: The Rust comment explains why deterministic aggregation is needed and keeps the CR1 formula nearby.
Concrete fix: None required.

Tech Debt

No findings.

Severity: N/A
Impact: No new deferred correctness limitation or untracked silent behavior change found.
Concrete fix: None required.

Security

No findings.

Severity: N/A
Impact: No secrets, unsafe code, or security-sensitive input handling changes found.
Concrete fix: None required.

Documentation/Tests

Severity: P3
Impact: The prior informational test gap remains. tests/test_rust_backend.py:L3647-L3657 imports diff_diff.linalg.compute_robust_vcov; that wrapper factorizes cluster IDs before Rust at diff_diff/linalg.py:L1785-L1790. So the test covers public API determinism for non-contiguous original labels, but not raw _rust_backend.compute_robust_vcov receiving non-contiguous unsorted IDs directly.
Concrete fix: Optional: add a small direct _rust_backend.compute_robust_vcov regression with raw IDs like [7, 3, 11, ...] and assert repeated-call bit identity.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 5aa51c71d909ea222f877fe49f8631c39e7f81de

---

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected method: Rust CR1 clustered sandwich vcov in rust/src/linalg.rs:342-L397, reached through compute_robust_vcov and solve_ols(return_vcov=True).
• Methodology matches the registry/docstring contract: CR1 meat remains Σ_g (X_g'u_g)(X_g'u_g)' with (G/(G-1))*((N-1)/(N-k)).
• The change removes HashMap iteration from the cluster-score row order; the map is now used only for deterministic cluster-index lookup.
• The prior P3 raw-kernel test gap is addressed by tests/test_rust_backend.py:L3661-L3676.
• I could not run tests locally: pytest/numpy are unavailable, and cargo check was blocked by restricted network access to crates.io.

Methodology

Finding: None.
Severity: N/A
Impact: The changed Rust path still implements the CR1 formula documented in docs/methodology/REGISTRY.md:L126-L128 and diff_diff/linalg.py:L1694-L1696. The meat and finite-sample adjustment remain unchanged at rust/src/linalg.rs:L385-L391; only row accumulation order changed.
Concrete fix: None required.

Code Quality

Finding: None.
Severity: N/A
Impact: HashMap randomized iteration no longer controls the (G, k) cluster-score matrix order. cluster_index is populated deterministically by first appearance and then used for direct row accumulation at rust/src/linalg.rs:L361-L383.
Concrete fix: None required.

Performance

Finding: None.
Severity: N/A
Impact: The implementation removes per-row to_owned() cluster sum allocation/replacement and accumulates directly into cluster_scores.
Concrete fix: None required.

Maintainability

Finding: None.
Severity: N/A
Impact: The Rust comment explains why deterministic aggregation matters and keeps the CR1 formula adjacent to the computation.
Concrete fix: None required.

Tech Debt

Finding: None.
Severity: N/A
Impact: No new deferred correctness limitation or silent behavioral change found.
Concrete fix: None required.

Security

Finding: None.
Severity: N/A
Impact: No secrets, unsafe code, or security-sensitive input handling changes found in the diff.
Concrete fix: None required.

Documentation/Tests

Finding: None.
Severity: N/A
Impact: The changelog documents the deterministic clustered-vcov fix at CHANGELOG.md:L38-L47. The prior informational gap is resolved: tests/test_rust_backend.py:L3661-L3676 directly exercises _rust_backend.compute_robust_vcov with unsorted, non-contiguous raw int64 cluster IDs.
Concrete fix: None required.