Audit: correctness, honesty, and a runnable test suite - #1
Merged
Conversation
Replay events were stamped and drained on performance.now() inside a loop paced by GPU throughput, so a shared link fired keys at different points in the cascade on any machine that ran the brain loop at a different rate. The README's "deterministically re-executes the identical neuron cascade" was not true. Stamp and drain on the brain's own clock instead: sim.currentStep advances by exactly BURST_STEPS per loop iteration, so events land on the same burst boundary on every host. dtMs is 1.0, so stamps stay 1:1 with simulated ms and the hold-duration recipe needs no conversion. The payload gains a 2-byte FNV-1a fingerprint of the DN roster, so a replay recorded against a different neuron list is rejected rather than silently misread. Pre-fingerprint links are 4+4N bytes and always fail the (len-6)%4 alignment check, which is what stops old millisecond stamps being reinterpreted as step counts. Every previously shared URL now warns and declines instead of replaying wrongly. Also: validate the decoded key index against dns[] (a crafted hash could index past it and kill the brain loop for the rest of the session), close out still-held keys at win so the encoded run has no unmatched down, count spikes during replay, and end playback on target reach without writing the viewer's local best score. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
step() pins freejoint linear and yaw qvel whenever a CPG command is set, and fwdCmd/turnCmd are only ever written by driveLegs() — which the trained-policy branch never calls. So after any DN stim, switching on the RL walker left the fly gliding at the CPG's last commanded speed no matter what the policy's 59 actions did, and the velocimeter/gyro channels fed back into the next observation were the assist's, not physics'. Clear both commands in applyTrainedWalkerActions, which has exactly one caller and runs immediately before each step(). The fly now crawls at the speed browser mujoco_wasm actually produces, which reads as slower but is the honest number. Also: reset() copied qpos_spring over every joint, re-introducing the out-of-distribution init that create() deliberately applies to the wing joints only — the two init paths now agree. And the MjVFS holding the mesh bytes is deleted once the model is compiled instead of being held for the life of the page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four ways a failure went unreported:
- A rejected await inside a stim handler left `busy` true forever, so
every button stayed disabled with no message. Wrap the handler bodies
in try/catch/finally. The continuous-loop toggle-off branch stays
outside the try, or stopping would clear `busy` while an iteration is
still in flight.
- With no WebGPU the boot overlay spun forever, because log() writes to a
pane that game mode hides. bootFail() writes to the overlay itself.
- brain.meta.json had a bare `catch {}` and no ok check, so a 404 removed
the famous-DN buttons and DN drive with zero diagnostics.
- A lost GPU device was unobservable; sim now reports it.
The large diff is re-indentation from the try blocks — `git diff -w`
shows 48 inserted lines.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cache key is the versioned URL, so every rebuild wrote a new entry and orphaned the old one — IDB evicts whole origins, never individual records, so a few rebuilds stranded hundreds of MB indefinitely. idbPut now drops other generations of the same asset inside its existing transaction. Also corrects two comments that described behaviour the code does not implement: cache.ts claimed hits are revalidated against HEAD/Content-Length (they are not; the ?v=<sha> in the key is the whole invalidation story), and brain.ts claimed old entries go under storage pressure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The header comment blamed a "silent-dispatch failure" on expanding the
Params uniform struct, and cited that as the reason A_SYN is a compile-time
const rather than a runtime field.
That is not what happens. Verified in Chrome with a minimal repro using
this exact 9-field struct and the same explicit bind-group layout
(minBindingSize unset): a 36-byte uniform buffer and a 48-byte one both
dispatch, read back correct values, and raise nothing under
pushErrorScope('validation'). The real silent-dispatch cause in this
project is the storage buffer count — the kernel binds 10 against a
default limit of 8, which is why FlySim.create requests the adapter's
limits.
The comment now states the actual constraint: A_SYN is const because the
kernel is fixed at dt = 1 ms. No WGSL behaviour changes; the point is that
the false diagnosis was blocking a legitimate future change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
upload_to_r2.sh never put flybody.bundle.bin or walking-ref.bin, yet both are live in the bucket and listed in assets.json — they were pushed out of band. A rebuild would therefore bump the manifest hash while R2 kept serving the old bytes, under a URL the cache treats as new content. Also in the pipeline: - build_vnc.py declared NT_CONF_MIN = 0.5 and never applied it, so vnc.bin weights were signed regardless of neurotransmitter prediction confidence, contradicting both the module comment and brain.bin's convention. This changes blob contents: it needs download_manc.sh, a rebuild, a fresh manifest and a re-upload to take effect, and the drive constants in main.ts were tuned against the current blob — re-verify walking after. - download_data.sh curled without -f, so an HTTP error page could be written as the 852 MB feather and the skip-if-exists guard would make that permanent. - download_manc.sh and download_flybody_policies.sh never cd to the repo root, so data landed relative to the caller's cwd. - build_csr.py resolved the Dallmann table relative to cwd while every other input uses ROOT, silently dropping the BPN preset; it now says so when the table is absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI ran typecheck and build only. The Playwright suite cannot run on a hosted runner — it needs WebGPU and ~300 MB of gitignored connectome binaries — so nothing in CI checked that the code did anything. Add five GPU-free, data-free unit checks over the VNC motor path and the walking observation layout, run with node --test (no new dependency). They live in tests-unit/ rather than tests/ so Playwright's testDir does not collect them, which avoids touching its config. Assertions are signs and structural invariants only: the LIF constants are emergent dynamics, not a contract, and asserting magnitudes would turn any legitimate retune into a red build. resetVnc() before each case is mandatory — the LIF state is module-level and mutable. Also: game-feel computed the two distances the test is named for and then discarded them, so it passed whether the fly walked toward the target or away from it. It now asserts the direction, with deliberate slack because open-loop Q can curve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This repo holds an explicit honesty bar, so a doc that overclaims is a defect. Measured against the live deployment and the code: - The replay claim is now stated precisely. The neuron cascade is reproducible (keystrokes re-fire at the same simulation steps against the same connectome and seeded target); the body trajectory is not, because physics advances a fixed substep count per animation frame and so depends on refresh rate. Said both, rather than deleting the claim. - "One fused GPU dispatch per timestep" was false — sim.ts issues two per step and three during capture. Reworded to name what is actually fused. - LIMITATIONS.md understated the edge count by ~3x, contradicting README, CITATION.cff and .zenodo.json. - CLAUDE.md's brain.bin size was ~45 MB; the asset on R2 is 125 MB. Its Kenyon-cell target (~1-5%) contradicted LIMITATIONS.md, the kernel comment and the test band. It documented `npm run test`, which does not exist, and pointed at machine-local absolute paths. - DEPLOY.md's env block omitted five of the nine VITE_ asset URLs, which is enough on its own to produce a broken production deploy, and its "set Cache-Control on R2" snippet was an invalid wrangler invocation that would have narrowed CORS instead. - README's Quickstart could not be completed: it pointed at a gitignored meshes.txt and never created the venv it later uses. The flybody mesh path is verified against TuragaLab/flybody upstream (85 .obj files). - vercel.json lacked the /assets/* immutable rule public/_headers has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every test except bench.spec.ts navigated to "/?mode=...". Since the landing page was split out of the simulator (6329233), "/" serves index.html, which has no #out log pane and never logs "game mode: ready" — so 33 of 34 tests sat in waitForFunction until their 120 s timeout. The suite has been vacuously failing ever since, which nothing noticed because CI cannot run it. Point them at /app, which resolves in both vite dev and the Pages deploy. With real connectome binaries in public/, 32 of 34 then pass. Also raise the famous-DN button visibility waits from 30 s to 60 s. Boot legitimately takes 35-45 s on a cold IDB profile (brain.bin, vnc.bin and the 140 MB flybody bundle all load before the buttons are built), so the 30 s wait was racing boot: the identical RRN and BPN tests would pass or fail depending on which side of the race that run landed. The button was always there — verified in the DOM. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reverts only the fwdCmd/turnCmd clearing from cd6827b; the reset() pose and MjVFS changes in that commit stand. The underlying finding is real and worth recording. step() pins freejoint qvel whenever a CPG command is set, and science mode auto-runs a preset at boot, so fwdCmd is already non-zero by the time anyone enables the trained walker. Clearing it exposes what the policy does on its own: measured over the same 4 s window, dx = -0.294 cm with the assist off versus dx = +1.227 cm with it on. The demo's forward walking under RL is the assist, not the policy. Not shipping that, because it is a product call rather than a defect fix. The assist is a deliberate, documented demo cheat with a user-facing "Honest mode" toggle that already turns it off, so forcing it off only on the RL path removes that choice and makes the flagship feature visibly fail to walk in the default view. tests/smoke.spec.ts:469 encodes the current expectation (dx > 0.1) and failed with the change in place. Left for the owner to decide, with the numbers above as the evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Multi-dimension audit of the repo (kernel, data loading, main loop, physics, game determinism, build/deploy, Python tooling, docs, tests), with every finding adversarially verified before it was acted on. 39 findings confirmed out of 53 candidates; ~30 applied here.
The ones that matter
Replay determinism was claimed but not real. Replay events were stamped and drained on
performance.now()inside a loop paced by GPU throughput, so a shared link fired keys at different points in the cascade on a different machine. Events now ride the brain's own step counter (sim.currentStep), which advances exactlyBURST_STEPSper iteration. The payload gained a 2-byte FNV-1a fingerprint of the DN roster, which doubles as the version tag. Every previously shared replay URL is now rejected — deliberate, and it is what stops old millisecond stamps being reinterpreted as step counts.upload_to_r2.shnever uploaded two assets the runtime requires —flybody.bundle.bin(140 MB) andwalking-ref.bin. Both are live on R2 and listed inassets.json, so they were pushed out of band; a rebuild would have bumped the manifest hash while R2 kept serving stale bytes.The e2e suite has been vacuous since the landing page was split out (6329233). Every test but
bench.spec.tsnavigated to/, which servesindex.html— no#outpane, so 33 of 34 sat inwaitForFunctionuntil timeout. Nothing noticed because CI cannot run them. Now pointed at/app: 34/34 pass locally against the real connectome.CI validated nothing. Typecheck + build only. Added five GPU-free unit checks over the VNC motor path and policy observation layout, run with
node --test, no new dependency.Plus:
busyflag never released on a rejected await (every button disabled forever, silently); boot overlay spun forever with no message when WebGPU is absent;brain.meta.jsonhad a barecatch {}; IndexedDB grew unbounded, orphaning ~300 MB per rebuild;NT_CONF_MINdeclared and never applied inbuild_vnc.py;curlwithout-fcould write an HTTP error page as the 852 MB feather permanently.Docs were corrected against measured values, not inferred ones:
brain.binis 125 MB (docs said ~45 MB), edges are 15,091,983 (LIMITATIONS.md said ~5M), "one fused dispatch per timestep" was false, and DEPLOY.md's env block omitted 5 of 9VITE_URLs — enough on its own to produce a broken deploy.Left for you to decide
revert(physics)documents a real finding I did not ship. With the kinematic assist correctly disabled during the trained-walker path, the fly moves backward 0.294 cm over 4 s; with it on, forward 1.227 cm. The demo's forward walking under RL is the assist running on a stale CPG command from the boot auto-stim, not the policy's 59 actions. The assist is a documented demo cheat with an existing "Honest mode" toggle, so forcing it off only on that path is a product call, not a defect fix.Also verified empirically and corrected in
lif.wgsl: the "expanding the Params uniform struct caused a silent dispatch failure" note is false. A 36-byte and a 48-byte uniform buffer both dispatch cleanly underpushErrorScope('validation'). The real cause is the storage-buffer count (10 bound vs a default limit of 8), already handled inFlySim.create. Adding aParamsfield is safe.Verification
tsc --noEmitclean ·npm run buildclean ·npm run test:unit5/5 ·npm run test:e2e34/34 against real R2 binaries.Not applied, with reasons in the audit: mocap walking-ref rate (needs an absent dataset), variable physics substeps per frame (changes tuned game feel), score normalization, cache HEAD-revalidation, download progress UI, naga WGSL validation in CI (naga vs Tint dialect drift → false reds).
🤖 Generated with Claude Code