path: root/design/reviews/codex-review.md

# Hostile reproducibility review — 2026-08-14

Scope reviewed: `src/`, `tests/`, and `design/preregistration.md`.  This is a code/design audit only; no training artifacts were assumed valid. `pytest` could not be run because it is not installed in the environment.

## Findings

### BLOCKER — Variable-length batches change the input seen by both models

`make_batch` right-pads inputs to the longest input in the batch.  In the tied RNN, `_encode` iterates across *all* padded positions and calls `_cell_step` even when `mask` zeroes the embedding/position contribution ([`src/models/rnn.py`](../../src/models/rnn.py), `_encode`). Thus a one-digit input receives additional tied transformations whenever it is batched with a two- or three-digit input.  At evaluation and greedy decoding, every example is passed as a singleton, so it receives no such transformations.  The same numerical input therefore has a batch-composition-dependent representation during training and a different representation at evaluation.

The transformer has the corresponding, and more severe, alignment error ([`src/models/transformer.py`](../../src/models/transformer.py), `forward`): logits always start at `T_in - 1`. For a shorter example in a mixed-length batch, this is a **padded input position**, not that example's final real input digit. Output positions are also shifted by the batch maximum input length, so their learned absolute positional embeddings differ from singleton evaluation. `key_padding_mask` prevents padding from being a key, but does not move the prediction position or remove its residual/query representation.

This silently makes both reported training behavior and validation/probe behavior depend on batching, and invalidates comparisons. Mask recurrent state updates by each sequence length and gather each transformer's first prediction at `length(x)-1` (or use a packing/layout that places each output immediately after its own input); train and evaluate with the identical layout. Add mixed-vs-singleton invariance tests for logits.

### BLOCKER — The implementation assigns preregistered O/H/P labels using unregistered and incorrect rules

`src/eval.py` claims to implement the locked codes but materially changes them.

* **O1:** `grokking_signature` permits the ten-evaluation train-saturation window to occur anywhere, including after the validation transition. The preregistration requires train EM to be sustained first, then the transition. It also classifies an early `val >= .9` result as O1 based on an unrelated earlier low validation point.
* **O2:** its stated condition is final validation EM `<= .3`; the code labels *any* run with `max(train) >= .95` and no `val >= .9` as O2, including a final validation EM of .31--.89. That directly contradicts O2 and hides an unclassified/ambiguous outcome.
* **H1--H4:** `halting_report` evaluates the selected checkpoint rather than the state “by run end,” declares H1 at mean `<= min_steps + .5` (default `<=2.5`) rather than collapse to 1, and declares H3 based solely on `abs(correlation) >= .3`. It neither checks the registered intermediate 2--18 range nor evolution during training; a negative correlation is treated as desired evidence. H2 uses an unregistered `K-.5` threshold. The logged scalar batch mean cannot establish the required per-input evolution either.
* **P1--P4:** the code invents thresholds absent from the locked matrix (`acc >= .85`, `easy_misses >=5`, and `<=6` errors). In particular P4 requires failure across the probe including easy cases, but five easy misses can be called P4 even at 95% accuracy; P3 is tested first, so it can also override that condition. The P2 catch-all does not test scattered/uniform composite errors. These labels are post-hoc classifications, not the preregistered outcomes.

Do not publish any O/H/P code from this evaluator. Either make each condition faithfully operational with thresholds added in a preregistration amendment before runs, or emit measurements plus `unclassified` where the prose does not supply a testable boundary. Preserve checkpoint/step provenance in every measurement.

### MAJOR — ACT `min_steps` is off by one, making the declared H1 condition impossible

In `_run_compute`, loop index `t=0` represents compute step 1, but halting probability is zeroed when `t < cfg.min_steps`. With the default `min_steps=2`, probabilities for steps 1 *and 2* are forced to zero, so the earliest non-remainder halt is step 3. The returned expected steps can never collapse to 1 (or even below 3), while the preregistration calls H1 “collapse to 1.” The test only asserts `steps >= min_steps`, which does not expose this error.

If `min_steps` means “execute at least two steps,” zero only the first `min_steps - 1` probabilities. Then align H1/H3 code definitions and tests with the chosen indexing convention.

### MAJOR — Validation is repeatedly used for checkpoint selection and then reported as the primary held-out result

There is no train/validation input overlap: `get_splits` uses one seeded shuffle and disjoint slices. Repeated next-prime *labels* across distinct inputs are inherent to the task, not input leakage. However, training evaluates the validation set every 200 updates, stops on it, and saves `best.pt` whenever its validation EM improves ([`src/train.py`](../../src/train.py), `_eval_pass`). `src/eval.py` defaults to that val-selected checkpoint and reports its validation EM as the primary result and its probe outcome. The same holdout therefore controls stopping/checkpoint selection and supplies the headline estimate; it is no longer an unbiased final estimate after hundreds of looks.

The early-stopping rule is preregistered, but the report must call this a validation-selected result, not an untouched held-out test result. For confirmatory performance, freeze a predeclared checkpoint rule (for example, the first qualifying stop) and use an untouched test split or independent seeds/replications; do not select the maximum observed validation EM.

### MAJOR — Rerunning a seed in place corrupts `metrics.csv` and breaks analysis provenance

The output path is deterministic (`runs/<model>/seed<seed>`), but `train.py` opens `metrics.csv` in append mode and unconditionally writes a header on each invocation. A second invocation interleaves another CSV header among old rows; `grokking_signature` then attempts `float("train_em")` and fails. If the new run has no improvement, a prior `best.pt` can also remain in place. Config JSON, metrics, and checkpoints can consequently originate from different invocations.

Require an empty/new run directory, or create a unique run ID and write a manifest atomically. Refuse to resume without explicit resume semantics that restore model, optimizer, step, and all RNG states.

### MAJOR — Determinism is only partial and not documented as CPU-only

`set_seed` seeds Python, NumPy, and PyTorch CPU RNGs, and the data shuffle has its own seeded `Random`; this is a good start. But it neither records/enforces the PyTorch version and threading environment nor enables deterministic algorithms or CUDA/cuDNN deterministic settings. Future device placement, GPU use, or nondeterministic kernels will therefore make same-seed runs diverge without warning. There is also no resume checkpoint for optimizer/RNG state.

Record environment/version/device metadata, explicitly select a device, and enforce deterministic settings appropriate to it. Test two fresh, isolated same-seed runs for byte-equivalent metric rows (or document a bounded tolerance). Keep each run in a fresh directory as above.

### MAJOR — `integers` vocabulary aliases EOS with a valid target and is not range-safe

Although the preregistered arm uses digit tokens, the supported `vocab_mode="integers"` is broken. At the default range end, token 101 is both the valid target `next_prime(100)` and `eos_id` (`vocab-1`); larger ranges can have a next prime beyond `range_end+1`, causing an embedding/target index failure. `decode_tokens` in integers mode ignores EOS entirely, which hides the alias. The existing test checks only output shape.

Reserve separate BOS/EOS/PAD IDs outside the full input/target integer domain, derive that domain from the actual maximum target, and add boundary tests. Until then, reject integer mode rather than silently producing invalid semantics.

### MINOR — ACT aggregation itself is normalized, but `halt_eps` is dead configuration

The ACT aggregation is mathematically normalized: `w_t = remaining * p_t`, `remaining *= 1-p_t`, followed by the final remainder on `h_list[-1]`; returned expected steps and its mean penalty are differentiable. Penalizing `mean(halt_steps)` with the scheduled `lambda` is coherent with that distributional definition and correctly disabled for fixed-K runs. There is no normalization bug here.

However, `Config.halt_eps` is documented as an ACT cumulative threshold but is never read. The code always computes all K states and uses a soft remainder, contrary to that configuration's description. Remove the unused setting or implement and test the specified behavior; do not imply a threshold is active in run configs.

### MINOR — Pad-target clamping is safe only because masking follows it, but lacks a guard/test

`y_safe = y.clamp(max=vocab-1)` maps pad ID to EOS solely to make cross-entropy indexable, then multiplication by `y_mask` removes those terms. With the current `make_batch`, this produces correct token loss and avoids pad embeddings in the output head. It is fragile: any future mask mismatch would silently train padded positions toward EOS. Assert that every masked-in target is `< vocab` and every masked-out target is exactly `pad_id`, or use an ignored-index loss. Add a test that perturbing logits at padded target positions cannot change loss or gradients.

### NIT — Tests do not exercise the reviewed failure modes

The suite checks shapes, a broad halting range, and finite backward passes, but not ACT mass summing to one, penalty gradients, mixed-length versus singleton behavior, evaluator-code boundary cases, fresh-run handling, or deterministic repeatability. These omissions allowed the issues above to pass. Also, `log_examples` orders with `len(str(x))` where `x` is a token list; it is deterministic but obscures the intended numeric ordering.