Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 863700acf6
ℹ️ 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".
Comment on lines
+65
to
+66
| let ulps = ordered_bits(a).wrapping_sub(ordered_bits(b)); | ||
| ulps.saturating_abs() <= 4 |
There was a problem hiding this comment.
For callers that are replacing float_cmp!(..., epsilon = e), this extra ULP fallback changes the old check: the named-argument macro started from a zero margin, so ulps was 0 unless explicitly supplied. In the simulation-result helpers, large-magnitude expected/actual values that differ by 1–4 ULPs but exceed the requested epsilon now pass, which can mask numeric regressions in VM/wasm or Vensim comparison tests instead of failing at the configured tolerance.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch -- confirmed against float_cmp 0.10's macros.rs: the named-argument form builds from Margin::zero(), so the original epsilon = e call sites ran with ulps = 0 and the ported 4-ULP fallback was a loosening. Fixed in 8c9c1d8: approx_eq_eps is now exact-equality OR within-epsilon OR bit-identical (the to_bits arm is what ulps == 0 meant -- it only adds identical-payload NaNs over a == b), with a regression test pinning that a few-ULP difference beyond the requested tolerance fails. The default-margin approx_eq keeps its 4-ULP arm, which is the genuine float_cmp default.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
A supply-chain audit found seven devDependencies that are declared but never referenced by any source file, build script, or eslint config: @rsbuild/plugin-node-polyfill and browserslist (app; rsbuild reads the browserslist package.json field with its own resolver), eslint-plugin-import, eslint-plugin-prettier (the eslint.config.shared.js setup uses eslint-config-prettier plus a separate prettier check instead), eslint-plugin-jsx-a11y, eslint-plugin-react-hooks, and @types/react-helmet (website; there is no react-helmet usage). Removing them drops about 90 distinct packages from the pnpm lockfile.
stringreader was an unused simlin-cli dependency (last published nine years ago). lazy_static was unused in simlin-engine (the codebase moved to std::sync::LazyLock); its one remaining use in xmutil was a static Mutex, which has been const-constructible since Rust 1.63 and needs no lazy initialization at all.
crate::float::approx_eq now implements the comparison directly instead of delegating to float_cmp: exact equality, then absolute difference within f64::EPSILON, then 4 ULPs over the sign-magnitude-to-monotonic bit mapping. The semantics are pinned byte-for-byte to float_cmp 0.10's default margin (new tests cover the near-zero epsilon arm, the ULP arm at magnitude, the sign-straddle case, and the deliberate MAX==+inf 1-ULP consequence) because the wasmgen approx_eq helper reproduces this exact algorithm and VM/wasm parity depends on it. approx_eq_eps carries the custom-epsilon form the integration test helpers used via float_cmp's macro. ordered-float's only use was the bytecode literal-interning key; interning needs Eq + Hash, not an ordering, so key on f64::to_bits() instead -- bit-exact dedup never conflates distinct values (at worst -0.0 and 0.0 get separate slots, where OrderedFloat collapsed them to one).
The ssh-key dev-dependency was used in exactly one place: extracting the raw 32-byte ed25519 key from an OpenSSH-format public-key blob in the ai_info verification tests. The relevant wire format (RFC 4253: a u32-be length-prefixed algorithm tag, then a length-prefixed key) is a dozen lines to parse directly, while ssh-key's default features pull in 22 crates including the full RSA and NIST P-256/384/521 stacks.
The cross-platform smoke test only speaks plaintext HTTP/1.1 to a loopback listener, and the crate's dev-dependencies already included hyper, hyper-util, and http-body-util for the other integration tests. A ~70-line helper over the hyper legacy connection pool replaces reqwest, removing 27 crates from the lockfile including the entire rustls/quinn/ring TLS stack that a loopback test never exercises. Verified by running the (normally #[ignore]d) smoke test end-to-end.
open_browser only ever launches one fixed per-OS command (xdg-open / open / cmd start) against our own loopback URL, which has no spaces or shell metacharacters -- the WSL/Flatpak/$BROWSER detection logic the open crate adds (plus is-docker and is-wsl) is surface area without benefit. The static-asset Content-Type lookup serves a closed set of extensions from the embedded SPA bundle, so an explicit table is more auditable than mime_guess's general-purpose database; unknown extensions keep the octet-stream fallback.
Replace each with the small slice of behavior we actually used:
- passport + passport-strategy: the only role was stashing {user: {id}}
in the session on login, deserializing it per request, and clearing it
on logout. session-auth.ts implements exactly that against the same
seshcookie-encrypted cookie, keeping the historic session.passport.user.id
wire shape so live logins survive the upgrade. This also deletes the
passport-0.7 session.regenerate/save compat shim from app.ts, which
existed solely for passport.
- winston: the server used four level functions; logger.ts emits the same
{level, message, timestamp} JSON lines (and renders Error stacks, which
winston serialized as a lossy {}).
- body-parser: express 5 re-exports the same implementation as
express.json()/express.urlencoded().
- serve-favicon: favicon.ts reads the icon once at startup and serves it
with the same caching and 405 semantics.
- uuid: node's crypto.randomUUID() is the same v4 format.
- @google-cloud/trace-agent: in maintenance mode upstream (superseded by
OpenTelemetry), and was a runtime monkey-patching agent we were not
actively reading traces from.
New modules carry their own unit tests; the passport/seshcookie compat
test is replaced by session-auth.test.ts covering the same login,
persistence, logout, and legacy-wire-shape scenarios.
fromUint8Array/toUint8Array were the only two js-base64 functions in use (encoding serialized project protobufs). @simlin/core/base64 implements them on the atob/btoa host globals (browsers and Node 16+), chunking the encode so large projects don't blow the String.fromCharCode argument limit, and keeping js-base64's tolerance for URL-safe alphabets and stripped padding on decode. The website copy of the dependency was only referenced from a documentation code snippet, now updated.
firebase-tools was a devDependency of @simlin/server used only by scripts/start-emulators.sh, yet its ~640-package closure (292 packages exclusive to it) was resolved, installed, and trusted by every developer and CI pnpm install. It now lives in tools/firebase-emulators/ -- standalone, outside the pnpm workspace, pinned by its own committed lockfile -- and start-emulators.sh installs it on demand (a no-op when already present), so only machines that actually run the emulators ever fetch it. The version stays exact-pinned rather than caret-ranged since nothing else co-resolves with it. This removes ~2,900 lines from the main pnpm-lock.yaml and shrinks the JS dependency graph by roughly 290 packages.
@simlin/app imports @firebase/app, @firebase/auth, and wouter at runtime, but listed them as devDependencies (it works because the app is bundled, but the manifest should describe reality); move them to dependencies. @simlin/serve-web declared @simlin/core and @simlin/engine but only ever imports @simlin/diagram, which depends on both; drop the unused direct edges. Audited the remaining packages: @simlin/diagram's slate-dom dependency is intentional (it satisfies slate-react's peer requirement), app's katex is used via a side-effect CSS import, and engine/core/server/ website declare exactly what they import.
Review caught a semantics drift in the float_cmp port: the named-argument macro form approx_eq!(f64, a, b, epsilon = e) builds its margin from Margin::zero(), not Margin::default(), so the original comparisons ran with ulps = 0. Porting approx_eq_eps with the default-margin 4-ULP arm made it more permissive than what it replaced: a simulation result beyond the requested tolerance but within 4 ULPs of the expectation would pass, silently absorbing small numeric regressions in the VM/wasm and Vensim comparison tests whenever the configured tolerance is tighter than ~2.2e-16 relative. approx_eq_eps is now exact-equality OR within-epsilon OR bit-identical (the to_bits arm is what ulps == 0 meant; over a == b it only adds identical-payload NaNs). The default-margin approx_eq is unchanged -- its 4-ULP arm is the documented float_cmp default the VM and wasmgen share.
Review summaryI reviewed this PR with a focus on the introduced replacement code: After a careful pass, I did not identify any discrete, actionable correctness bugs introduced by this change. Notes on the trade-offs that are already documented in code:
Overall correctnessThe patch appears correct. Tests have been added for each replacement (base64 round-trip, formatLogEntry, favicon, session-auth, float approx_eq with NaN/ULP/sign-straddle/epsilon coverage), and the unchanged production algorithms (wasmgen |
bpowers
added a commit
that referenced
this pull request
Jun 21, 2026
## Summary A follow-up to PR #719: a fresh attribution audit of both lockfiles (Rust workspace crates + all JS packages), verifying actual call sites before cutting. Everything removed is dead weight, a redundant pin, or replaceable by a small amount of tested code we own. | | Before | After | Delta | |---|---|---|---| | Cargo.lock crates | 467 | 463 | -4 | | pnpm-lock packages | 1346 | 1320 | -26 | The headline counts undersell it: the biggest wins are deduplicating parallel transitive trees (firestore/gax) and evicting the `@floating-ui`/popper positioning stack, both of which trade many duplicated/heavy packages for owned code. ## server: redundant firestore pin + dead cookie-parser `firebase-admin` already bundles `@google-cloud/firestore` and re-exports its full type surface (plus an app-aware `getFirestore()`) from `firebase-admin/firestore`. The server *also* pinned `@google-cloud/firestore@^8.3.0`, which resolved to `8.5.0` next to firebase-admin's `7.11.6`; the two diverge below `google-gax` (4.x vs 5.x), dragging parallel copies of `google-gax`, `google-auth-library`, the grpc proto loader, and `node-fetch` into the tree. Retargeted the two import sites at `firebase-admin/firestore` and swapped `new Firestore()` for `getFirestore()` (the admin app is already initialized before the DB is built). The runtime API used (collection/doc/where/runTransaction, `FieldPath.documentId()`) is stable across 7.x/8.x. `cookieParser()` was registered as middleware but nothing reads `req.cookies`/`req.signedCookies` (seshcookie parses the Cookie header itself). Removed it + `@types/cookie-parser` and the duplicate `cookie-signature` copy it pulled. ## build: criterion plotters trim + unused tracing `criterion` is now `default-features = false, features = ["cargo_bench_support"]`, dropping the `plotters`/`plotters-backend`/`plotters-svg`/`web-sys` stack (we only read the terminal summary; the bench harnesses report in-chat). Removed the declared-but-unused `tracing` dependency from `simlin-mcp-core` (Closes #803). ## diagram: reimplement 4 UI components, drop 5 deps Replaced five third-party UI dependencies with small owned implementations, keeping each component's public API and its existing tests as the contract. This **fully evicts the `@floating-ui/*` + `react-popper` positioning stack** and `downshift`. - **Accordion** -- context-backed single-item disclosure; the open/close height animation is a JS-free `grid-template-rows` `0fr<->1fr` transition (no more Radix `--radix-accordion-content-height`). Adds an Accordion test where none existed. - **Checkbox** -- native `<input type="checkbox">` (appearance:none) with a CheckIcon overlay, preserving the `data-state` + primary/secondary class contract. - **Autocomplete** -- a new owned `useCombobox` hook (filter-as-you-type listbox, arrow/enter/escape keys, WAI-ARIA combobox attributes) replaces downshift's `useCombobox`; the component body is otherwise unchanged. - **Menu** -- an owned fixed-position portal positioned from the live anchor rect (the `useAnchorRect` issue-#710 logic is unchanged), with Escape + click-outside dismissal and close-on-select via context. Drops Radix dropdown-menu (and with it `@floating-ui`). - **SpeedDial action tooltips** -- a CSS-only hover/focus tooltip replaces Radix tooltip. Radix `dialog`/`tabs`/`toast` deliberately stay -- they carry focus-trap/ARIA correctness that is not worth re-owning. All 1195 diagram tests pass. ## Deliberately kept (audited, not worth touching) - **Rust**: `ignore`, `calamine`, `notify-debouncer-full`, `jsonschema` -- big headline closures evaporate under inverse-dependency analysis (shared with `tracing-subscriber`/`salsa`/`loro`) or are already correctly feature-gated. `twox-hash`/`smallvec`/`rustc-hash`/`indexmap` add zero net crates. - **JS**: `helmet`/`@iarna/toml` (zero deps), `cors` (one tiny pkg), `katex` (one call site, but deps only on `commander` -- weight is fonts not tree), `slate`, `@firebase/*`, `clsx`, `wouter` -- load-bearing or zero-closure. ## Verification Every commit passed the full pre-commit battery (rustfmt, clippy, cargo test, eslint, build, tsc, JS tests, pysimlin). New/reimplemented components keep their existing component tests green; the firestore retarget and cookie-parser removal were validated against the server suite (111 tests) and tsc. Closes #803 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
Summary
A supply-chain audit of both lockfiles, attributing transitive closure sizes to each direct dependency and verifying actual usage at every call site. Everything removed is either dead weight (declared but never referenced), replaceable by a small amount of code we own, or (firebase-tools) relocatable out of the standing graph.
Dead weight (zero behavior change)
@rsbuild/plugin-node-polyfill,eslint-plugin-import,eslint-plugin-prettier,eslint-plugin-jsx-a11y,eslint-plugin-react-hooks,browserslist(app),@types/react-helmet(website). The react-hooks plugin is worth wiring in for real someday; that's now tracked separately.stringreader(simlin-cli, unused),lazy_static(engine already usedstd::sync::LazyLock; xmutil's one use was a staticMutex, const-constructible since Rust 1.63).Implemented ourselves (small, tested, owned)
crate::float::approx_eqnow implements the ULP comparison directly (pinned byte-for-byte to float_cmp 0.10's default margin -- the wasmgen backend reproduces this algorithm, so VM/wasm parity depends on exact semantics). Bytecode literal interning keys onf64::to_bits()instead ofOrderedFloat.ssh-key(-22 crates incl. the RSA + NIST P-curve stacks).reqwest(-27 crates incl. rustls/quinn/ring);open_browseris one fixed per-OS command; static-asset Content-Type is an explicit table over the closed set of embedded SPA extensions.base64.ts(atob/btoa with chunked encode) replacesjs-base64in app/diagram/website.session-auth.tsreplaces passport (preserving thesession.passport.user.idcookie wire shape so live logins survive);logger.tsreplaces winston (same JSON line shape, plus Error stacks); express 5's built-injson()/urlencoded()replace body-parser; a small middleware replaces serve-favicon;crypto.randomUUID()replaces uuid; the deprecated@google-cloud/trace-agentruntime-patching agent is dropped.firebase-tools out of the standing graph
Its ~640-package closure existed so
start-emulators.shcould find the binary. It now lives intools/firebase-emulators/-- standalone, outside the pnpm workspace, pinned by its own committed lockfile -- and is installed on demand only on machines that run the emulators. This alone removes ~2,900 lines from the main pnpm-lock.yaml.deps vs devDeps tightening
@simlin/appruntime imports (@firebase/app,@firebase/auth,wouter) move from devDependencies to dependencies;@simlin/serve-webdrops@simlin/core/@simlin/engineedges it never imports directly. Audited the rest: diagram'sslate-domstays (satisfies slate-react's peer), app'skatexstays (side-effect CSS import).Verification
#[ignore]d simlin-serve smoke test was run end-to-end against the hyper rewrite.Deliberately untouched:
seshcookie's express@4 pin (being handled separately), and the big load-bearing deps (loro, resvg, rmcp, axum, salsa, tokio, react, jest).