chore(deps): fix OTel version split, drop dead deps, declare emcn peers - #5994
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview FFmpeg no longer uses Dependency cleanup in
Reviewed by Cursor Bugbot for commit 15f9e3f. Configure here. |
Greptile SummaryDependency hygiene across the monorepo with targeted runtime fixes.
Confidence Score: 5/5This PR appears safe to merge; no blocking failures remain from prior or new review findings. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/package.json | Pins OTel stack exactly, adds core and AppConfig client, removes ffmpeg-static/json5/critters and inert overrides, bumps lucide-react and image-size. |
| packages/ts-sdk/src/index.ts | Drops node-fetch for global fetch and folds error.cause into SimStudioError messages on network failures. |
| packages/emcn/package.json | Declares previously undeclared Radix/tanstack peers (and matching devDeps) plus @sim/utils dependency. |
| apps/sim/lib/audio/extractor.ts | Resolves FFmpeg via system PATH only after removing the unused ffmpeg-static path. |
| apps/sim/stores/variables/store.ts | validateVariable uses JSON.parse instead of JSON5; validationError remains write-only per PR intent. |
| package.json | Clears root dependencies and yaml; keeps trustedDependencies limited to isolated-vm and sharp. |
Reviews (2): Last reviewed commit: "fix(deps): complete the OTel pin, restor..." | Re-trigger Greptile
- Pin @opentelemetry/{resources,sdk-metrics,sdk-trace-base,sdk-trace-node}
to exact 2.7.1 so they match sdk-node's pins instead of floating to 2.8.0.
The carets meant app code built spans with 2.8.0 and passed them into
NodeSDK from 2.7.1, which only worked by duck-typing.
- Declare the 13 packages @sim/emcn imports but never declared, as peers
mirrored into devDeps. @radix-ui/react-dismissable-layer had no
declaration anywhere in the repo and resolved only transitively.
- Remove ffmpeg-static: its binary downloads via postinstall, but it is not
in trustedDependencies and Docker installs with --ignore-scripts, so the
accessSync branch never succeeded and both call sites always fell through
to system ffmpeg.
- Remove critters + experimental.optimizeCss: Next only loads critters from
the Pages Router renderer, and apps/sim is App Router only.
- Make simstudio-ts-sdk zero-dependency by dropping node-fetch for native
fetch; engines >=18.
- Remove unused @vercel/og and postgres from docs, dotenv/inquirer/listr2
from the CLI, and yaml from the root.
- Move @aws-sdk/client-appconfig from the root to apps/sim, its only consumer.
- Delete the apps/sim overrides block; Bun only honors top-level overrides.
- Bump free-email-domains 1.2.25 -> 1.9.70 (4,779 -> 13,059 domains).
- Validate object/array variables with JSON.parse instead of JSON5, matching
what the executor actually parses.
- Swap the changelog GitHub icon off lucide to GithubOutlineIcon, matching
the navbar chip on the same page.
- Unify @types/node on 24.2.1 and lucide-react on ^0.511.0; bump chalk to 5
and image-size to 2.
…ail list Follow-ups from an independent audit of the previous commit. - Pin @opentelemetry/sdk-node and the three otlp-http exporters to exact 0.217.0. Pinning only their four dependents was self-reversing: sdk-node 0.219.0 requires core 2.8.0 exactly, so the next update would have silently rebuilt the split this PR removes. - Declare @opentelemetry/core (2.7.1). It is imported by lib/copilot/request/go/propagation.ts but resolved only by hoisting, and it is the OTel package with the most version churn in the tree. - Pin @radix-ui/react-dismissable-layer to exact 1.1.13 in @sim/emcn. All five transitive parents pin it exactly; a caret would fork a second copy on 1.1.14, which is the duplicate-context bug the declaration prevents. - Surface error.cause in simstudio-ts-sdk. Native fetch reports network failures as a bare "fetch failed" and puts the reason on cause, so every DNS/TLS/refused error was reaching callers with no diagnostic content. - Revert free-email-domains to 1.2.25. Upstream now merges the free-domain list with two disposable-email blocklists, so 1.9.70 classifies real organization domains as free — UK charities, some companies and universities, and the JP/KR ISP domains APAC SMBs use for business mail. The demo form blocks submission on that check, so a false positive costs the booking entirely. Worth doing deliberately, not inside a deps change. - Lower packages/cli engines to >=18. chalk 5 and commander 11 both accept >=16 and the source uses no Node 20 API, so >=20 only produced EBADENGINE for Node 18 users.
eff2512 to
15f9e3f
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 15f9e3f. Configure here.
Summary
Dependency hygiene across the monorepo. Independently re-audited after the first round; corrections are in the second commit.
The real fix — OpenTelemetry was running two copies of the trace SDK.
^2.7.0onresources/sdk-metrics/sdk-trace-base/sdk-trace-nodefloated to 2.8.0 whilesdk-node@0.217.0pins those siblings at exactly 2.7.1, soinstrumentation-node.tsbuilt spans, sampler and resource from 2.8.0 and passed them intoNodeSDKfrom 2.7.1. It worked only by duck-typing, the gap widened on every install, and the failure mode is silent (spans stop exporting, sampler ignored) becauseinitializeOpenTelemetryswallows init errors into a log line. All four are now exact 2.7.1, andsdk-nodeplus the three otlp-http exporters are pinned exact too — pinning only the leaves was self-reversing, sincesdk-node@0.219.0requires core 2.8.0 exactly. Also declares@opentelemetry/core, whichlib/copilot/request/go/propagation.tsimports but resolved only by hoisting.@sim/emcnimports but never declared, as peers mirrored into devDeps.@radix-ui/react-dismissable-layerhad no declaration anywhere in the repo and resolved only transitively off five other Radix packages; it is pinned exact1.1.13because all five parents pin it exactly and a caret would fork a second copy on 1.1.14 — the duplicate-context bug the declaration exists to prevent.ffmpeg-static(dead). The binary downloads via an install script, but declaringtrustedDependenciesat all replaces Bun's default allowlist, so it is never fetched on any install path — verified with controlled installs. Both call sites always fell through to system ffmpeg, which the runtime image apt-installs. Note: this also drops an undocumentedFFMPEG_BINenv override thatffmpeg-static/index.jshonored (zero occurrences repo-wide).critters+experimental.optimizeCss(dead). Next only loads critters from the Pages Router renderer;apps/simis App Router only, so the flag never inlined critical CSS.simstudio-ts-sdkzero-dependency: native fetch,engines >=18. Native fetch reports network failures as a barefetch failedwith the reason oncause, so the SDK now foldscauseinto the message — without that, every DNS/TLS/refused error reached callers with no diagnostic content.@vercel/og+postgresfrom docs,dotenv/inquirer/listr2from the CLI,yamlfrom the root. Move@aws-sdk/client-appconfigtoapps/sim, its only consumer; rootdependenciesis now empty.apps/simoverrides block — Bun only honors top-level overrides, so it was inert (react-floaterisn't even in the lockfile).json5. Its only use wasvalidateVariable, whose result is written toVariable.validationError— a field nothing reads. This removes a dependency used by dead code; it does not change what users see.@types/nodeon24.2.1(6 installed versions → 4),lucide-reacton^0.511.0,chalk→ 5 in the CLI (engines >=18),image-size→ 2.GithubOutlineIcon, matching the navbar chip on the same page. Requested separately —lucide@0.511still exportsGithub; only v1 removes it.Type of Change
Testing
Type-check 19/19 and
sim14,803 passed, identical before and after on the same tree. The one failing test (cloud-review-tools.test.ts) needs a realrgbinary on PATH and fails identically on an untouched checkout.biomeclean,check:api-validationpasses,bun install --frozen-lockfileclean,apps/docsbuilds green (4,156 pages). CLI verified with byte-identical--help/--version/ANSI captures across the chalk 5 migration; ts-sdk tests mutation-checked (renaming an endpoint and a header produces failures, so the rewritten mocks are live).Lockfile: 168 entries removed, 49 added. Beyond the intended bumps, 17 further top-level resolutions moved as hoisting inversions — removing the CLI's
inquirer@8/listr2@6and ts-sdk'snode-fetch@3let newer transitive copies hoist (top-levelnode-fetch3.3.2 → 2.7.0, the ansi stack modernizing). Every prior consumer still resolves to its original version via re-nesting, and no first-party file imports any of the 17.Reverted after audit: the
free-email-domainsbump. Upstream now merges the free-domain list with two disposable-email blocklists, so newer versions classify real organization domains as free — UK charities, some companies and universities, and the JP/KR ISP domains APAC SMBs use for business mail. The demo form blocks submission on that check, so a false positive costs the booking entirely. Worth doing deliberately, not inside a dependency change.Checklist