feat(sentry): add release tracking - #204
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
- Coverage 66.33% 65.60% -0.73%
==========================================
Files 13 13
Lines 398 407 +9
Branches 121 127 +6
==========================================
+ Hits 264 267 +3
- Misses 91 92 +1
- Partials 43 48 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gitstream-cm[bot]](https://avatars.githubusercontent.com/in/230441?s=60&v=4){kind=small}
There was a problem hiding this comment.
✨ PR Review
LGTM
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Main concerns are around runtime compatibility and future-proofing: importing ../package.json in src/index.ts can break depending on the build/ESM environment, and the webhook changes mix two competing strategies (optional methods + presence guards vs always-present throwing stubs). The webhook stub return types (Promise<unknown>) plus truthiness checks risk subtle behavior changes when real endpoints are reintroduced. Workout test changes around description: undefined vs null may hide meaningful contract differences.
Additional notes (1)
- Readability |
src/tools/workouts.test.ts:207-215
Tests changeddescriptionfromnulltoundefinedin one case. IfformatWorkout(...)or downstream consumers distinguish betweennull(explicitly absent) andundefined(missing), this can mask a real API contract difference. Since this suite is intended to validate mapping/formatting, it’s better to assert the behavior you actually want (and keep it consistent across tests).
Summary of changes
What changed
Sentry release tracking
- Adds a
releasevalue toSentry.init(...)insrc/index.ts, derived fromprocess.env.SENTRY_RELEASE ?? \${name}@${version}``. - Updates the README Sentry section to mention the release name derived from the package version.
Webhook client/type alignment
- Updates
src/tools/webhooks.tsand its tests to treat webhook client methods as optional and guard on presence before calling. - Replaces previously referenced generated webhook endpoints in
src/utils/hevyClientKubb.tswith explicit throwing stubs (getWebhookSubscription,createWebhookSubscription,deleteWebhookSubscription) until the OpenAPI client is regenerated.
Workout request body refactor
- Refactors workout request-body construction in
src/tools/workouts.tsto build aworkoutPayloadobject and then wrap it asPostWorkoutsRequestBody. - Updates workout tests to use ISO timestamp strings for
start_time/end_timeand adjusts one case fromdescription: nulltodescription: undefined.
| import dotenvx from "@dotenvx/dotenvx"; | ||
| import * as Sentry from "@sentry/node"; | ||
| import { name, version } from "../package.json"; | ||
|
|
||
| // Configure dotenvx with quiet mode to prevent stdout pollution in stdio mode | ||
| dotenvx.config({ quiet: true }); | ||
|
|
||
| // Sentry monitoring is baked into the built MCP server so usage and errors | ||
| // from users of the published package are captured for observability. | ||
| const sentryRelease = process.env.SENTRY_RELEASE ?? `${name}@${version}`; | ||
| const sentryConfig = { | ||
| dsn: "https://ce696d8333b507acbf5203eb877bce0f@o4508975499575296.ingest.de.sentry.io/4509049671647312", | ||
| release: sentryRelease, | ||
| // Tracing must be enabled for MCP monitoring to work |
There was a problem hiding this comment.
Importing name/version from ../package.json inside the runtime entrypoint is a bundler/Node-ESM footgun: it can break depending on how the package is built/consumed (JSON module support, type: module, TS/packager settings), and it also increases the chance that the runtime release diverges from the actual deployed artifact version (e.g., packed tarball vs workspace). For release tracking, it’s usually safer to rely on build-time injection or an explicit env var for published builds.
Suggestion
Prefer a build-time constant or package metadata that’s known to be available at runtime.
Option A (recommended): require SENTRY_RELEASE for published builds and keep a dev fallback:
const sentryRelease = process.env.SENTRY_RELEASE ?? "dev";Option B: inject version at build time (via your build tool) and avoid JSON imports in runtime code:
declare const __APP_VERSION__: string | undefined;
const sentryRelease = process.env.SENTRY_RELEASE ?? `hevy-mcp@${__APP_VERSION__ ?? "dev"}`;Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing Option A (minimal runtime risk) or Option B (build-time injection) based on your preference.
There was a problem hiding this comment.
@CharlieHelps we are using tsup and we can inject build time variables
There was a problem hiding this comment.
Reviewing the feedback on PR #204 about using build-time variables with tsup in src/index.ts and adjusting my earlier assumptions.
Updated: Dec 20, 13:51 UTC
I won’t see any replies while I’m working on this and I can’t pause or stop until the task is finished.
There was a problem hiding this comment.
Expand this to see my work.
- Previously inspected
tsup.config.tsandsrc/index.ts/src/cli.ts, wired__HEVY_MCP_NAME__/__HEVY_MCP_VERSION__viadefine, removed the runtimepackage.jsonimport, added guarded parsing plus a/dist/-based runtime check, and verified withpnpm run build,pnpm run check:types,pnpm vitest run --exclude=tests/integration/**, andpnpm run check, landing commits 69a00f8, 31a98e2, and abebe68 onai-203-sentry-release-trackingfor PR #204. - Re-reviewed the maintainer comment on
src/index.tsin PR #204 about preferringtsup-injected build-time variables overimport.meta.url.includes("/dist/")for Sentry invariants. - Updated
src/index.tsto declare__HEVY_MCP_BUILD__alongside__HEVY_MCP_NAME__/__HEVY_MCP_VERSION__and replaced the string-basedisBuiltArtifactheuristic withconst isBuiltArtifact = typeof __HEVY_MCP_BUILD__ === "boolean" ? __HEVY_MCP_BUILD__ : false;so the guard is driven by an explicit build flag. - Extended
tsup.config.tsto inject__HEVY_MCP_BUILD__: "true"in thedefineblock next to__HEVY_MCP_NAME__and__HEVY_MCP_VERSION__, keeping the Sentry release metadata contract tied to the bundler configuration. - Ran
pnpm run check(Biome) after wiring__HEVY_MCP_BUILD__and confirmedChecked 40 files in 66ms. No fixes applied.indicating lint/format are clean. - Committed the build-flag wiring changes in
src/index.tsandtsup.config.tsasfix(sentry): use tsup build flag for release metadata(20d6ede) onai-203-sentry-release-trackingfor PR #204. - Pushed 20d6ede so PR #204 now reflects the
__HEVY_MCP_BUILD__-based Sentry guard on GitHub. - Confirmed via
git rev-parse HEAD/git log -5 --onelinethat20d6edeis the current head onai-203-sentry-release-tracking, matching the commit referenced in the planned review reply. - Used
gh pr edit 204 --add-reviewer CharlieHelpsto ensureCharlieHelpsis listed as a reviewer on PR #204 after checking existingreviewRequestswithgh pr view 204 --json reviewRequests. - Re-read the maintainer’s latest reply in the
src/index.tsthread abouttsupbuild-time injection and aligned my upcoming GitHub response with the current implementation intsup.config.tsandsrc/index.ts.
There was a problem hiding this comment.
Switched the Sentry release (and MCP server name/version) to tsup build-time injection instead of importing package.json at runtime. SENTRY_RELEASE still wins when set. Changes are in 20d6ede.
Changes
src/index.ts: use build-time globals (__HEVY_MCP_NAME__,__HEVY_MCP_VERSION__) forSentry.init({ release })+new McpServer({ name, version }), with a__HEVY_MCP_BUILD__flag so we fail fast if a built artifact is missing injected metadata.tsup.config.ts: parse + validatepackage.jsonat build time and inject__HEVY_MCP_BUILD__,__HEVY_MCP_NAME__,__HEVY_MCP_VERSION__viadefine.
Verification
# Build: success
pnpm run build
# Unit tests (integration excluded): 69 passed (13 files)
pnpm vitest run --exclude=tests/integration/**
# Biome: no errors (40 files)
pnpm run check
# TypeScript: no errors
pnpm run check:typesSelf-review notes (not changed):
src/index.ts: kept the thrown “missing build-time variables” error message short (follow-up could add more remediation hints).tsup.config.ts: didn’t add an inline comment about thepackage.jsonadjacency assumption (seems obvious in this repo layout).
| type HevyClient = ReturnType< | ||
| typeof import("../utils/hevyClientKubb.js").createClient | ||
| >; | ||
| > & { | ||
| getWebhookSubscription?: () => Promise<unknown>; | ||
| createWebhookSubscription?: (data: { | ||
| webhook: { | ||
| url: string; | ||
| authToken: string | null; | ||
| }; | ||
| }) => Promise<unknown>; | ||
| deleteWebhookSubscription?: () => Promise<unknown>; | ||
| }; |
There was a problem hiding this comment.
The webhook tools now treat webhook methods as optional, but src/utils/hevyClientKubb.ts always supplies these methods (throwing stubs). This makes the presence checks effectively dead code for the default client, and creates inconsistent behavior across implementations: a client that omits the method yields a custom “not available” error, while the default client yields a thrown error from inside the stub. That divergence can complicate tests/observability and makes the API surface ambiguous (is it optional or not?).
Suggestion
Pick one strategy and make it consistent:
-
Non-optional methods: keep the methods required in the
HevyClienttype and rely on the default client stubs to throw the clear error; remove theif (!hevyClient.<method>)checks. -
Truly optional methods: remove the stubs from
createClient()so missing methods are actually possible, and keep the presence checks.
Given you already added stubs with a clear message, (1) is simpler and avoids split behavior.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit to make the webhook methods non-optional and remove the redundant guards (or alternatively remove the stubs and keep guards).
| getWebhookSubscription: async (): Promise<unknown> => { | ||
| throw new Error( | ||
| "Webhook subscription API not available. Please regenerate the client from the updated OpenAPI spec.", | ||
| ); | ||
| }, | ||
| createWebhookSubscription: async ( | ||
| _data: CreateWebhookSubscriptionRequest, | ||
| ): Promise<unknown> => { | ||
| throw new Error( | ||
| "Webhook subscription API not available. Please regenerate the client from the updated OpenAPI spec.", | ||
| ); | ||
| }, | ||
| deleteWebhookSubscription: async (): Promise<unknown> => { | ||
| throw new Error( | ||
| "Webhook subscription API not available. Please regenerate the client from the updated OpenAPI spec.", | ||
| ); | ||
| }, |
There was a problem hiding this comment.
The webhook client stubs return Promise<unknown>, and the tool code treats truthiness (if (!data)) as meaning “no subscription/failed”. With the current stubs this path never happens (they throw), but once real endpoints are reintroduced, unknown + truthiness checks can easily encode the wrong semantics (e.g., a valid empty object, or a deletion endpoint returning {}/204 mapped to something falsy). This is setting you up for subtle behavior changes when the OpenAPI client is regenerated.
Suggestion
Make the contract explicit even while stubbing so tool behavior won’t drift when the real client arrives.
- If you expect an object response, stub with a stable shape and document it:
type WebhookSubscriptionResponse = { webhook?: { url: string; authToken: string | null } } | null;
getWebhookSubscription: async (): Promise<WebhookSubscriptionResponse> => {
throw new Error("...");
}- Or, if the endpoint is expected to be
204, have the tool not rely ondatatruthiness and instead treat success as “no exception”.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit that refactors the webhook tools to not use if (!data) for delete/create (success-by-no-throw) and tightens the stubbed return types to match the intended semantics.
There was a problem hiding this comment.
The main risk is inconsistent and fragile webhook semantics: methods are treated as optional in the tools while the default client always provides throwing stubs, and if (!data) checks combined with Promise<unknown> are likely to break or drift when real endpoints return 204/empty payloads. Tests also now mix null vs undefined for description, which can mask meaningful contract differences. Sentry release injection is improved, but the current fallbacks can silently emit @dev releases in production-like runs if injection fails.
Additional notes (2)
- Compatibility |
src/tools/webhooks.ts:73-73
These handlers useif (!data)to determine success/failure for webhook operations. With today’s throwing stubs this is dead code, and when real endpoints return204or{}this can produce incorrect behavior (e.g., treating a successful delete that returns nothing as failure).
For create/delete, success is typically “request completed without throwing”, not “returned a truthy body”.
- Readability |
src/tools/workouts.test.ts:207-215
This test change switchesdescriptionfromnulltoundefined. That’s a behavioral difference that can matter in mapping/formatting layers and in API contracts (explicitly absent vs omitted). If the intent is “no description”, the suite should assert one canonical representation consistently.
Right now, the implementation uses description || null when sending requests, but this test is about the response formatting path—mixing null/undefined in fixtures can hide contract drift.
Summary of changes
Summary of changes
Sentry release tracking
- Added build-time injected globals
__HEVY_MCP_NAME__/__HEVY_MCP_VERSION__with runtime fallbacks insrc/index.ts. - Configured
Sentry.init(...)withrelease: process.env.SENTRY_RELEASE ?? \${name}@${version}``. - Updated
README.mdto document release naming derived from package version.
Build configuration
- Enhanced
tsup.config.tsto parse and validatepackage.json(name,version) and inject them viadefineas__HEVY_MCP_*constants.
Webhook tooling compatibility
- Updated webhook tool and tests to treat
getWebhookSubscription/createWebhookSubscription/deleteWebhookSubscriptionas optional methods and to guard before calling. - Replaced missing generated webhook endpoints in
src/utils/hevyClientKubb.tswith explicit throwing stubs.
Workout payload construction
- Refactored workout create/update request-body construction to build a
workoutPayloadfirst, then wrap as{ workout: workoutPayload }. - Adjusted tests to use ISO timestamps for
start_time/end_timeand changed onedescriptionassertion fromnulltoundefined.
There was a problem hiding this comment.
✨ PR Review
The PR successfully adds Sentry release tracking with proper build-time variable injection and improves type safety by removing as any casts in webhook handling. The implementation is well-structured with appropriate fallbacks for development environments.
1 issues detected:
🧾 Readability - Error message doesn't match the actual validation condition being checked 🛠️
Details: The error message states that build-time variables "must be defined" but the actual validation checks if they are strings, not just defined. This could confuse developers debugging build issues.
File:src/index.ts (15-17)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
| throw new Error( | ||
| "Build-time variables __HEVY_MCP_NAME__ and __HEVY_MCP_VERSION__ must be defined.", | ||
| ); |
There was a problem hiding this comment.
🧾 Readability - Misleading Error Message: Update the error message to accurately reflect that the variables must be strings, e.g., "Build-time variables HEVY_MCP_NAME and HEVY_MCP_VERSION must be strings."
| throw new Error( | |
| "Build-time variables __HEVY_MCP_NAME__ and __HEVY_MCP_VERSION__ must be defined.", | |
| ); | |
| throw new Error( | |
| "Build-time variables __HEVY_MCP_NAME__ and __HEVY_MCP_VERSION__ must be strings.", | |
| ); |
Is this review accurate? Use 👍 or 👎 to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
There was a problem hiding this comment.
The biggest concern is fragile webhook semantics: optional-method guards conflict with always-present throwing stubs, and if (!data) checks are likely to mis-handle real-world webhook endpoints that return 204/empty bodies. The Sentry release injection is on the right track, but the current guard/error messaging is slightly misleading and could still allow silent @dev releases if build injection fails. tsup.config.ts error output could be more actionable for build failures. Workout tests mixing null vs undefined for description risks masking contract differences.
Additional notes (3)
- Maintainability |
src/tools/webhooks.ts:73-73
The tool usesif (!data)to determine success for create/delete/get webhook subscription. With the current default client, these methods always throw (stubs), so the falsy checks are effectively dead. When real endpoints are regenerated, webhook delete commonly returns 204 / empty payload, and create may return{}-like responses—both can be falsy/truthy in surprising ways depending on the client wrapper.
This is fragile API semantics: success should generally be “call completed without throwing”, not “returned a truthy body”.
-
Readability |
tsup.config.ts:22-33
Thetsup.config.tserror message usesString(name)/String(version)which can collapseundefined/nullinto ambiguous strings and loses detail for objects. Since this error is meant to diagnose build-time metadata injection failures, it should be as actionable as possible (include type info and a clear remediation). -
Maintainability |
src/tools/workouts.test.ts:207-215
This test fixture changesdescriptionfromnulltoundefined. That’s a behavior difference that can matter in mapping layers and API contracts (explicitly empty vs omitted). Since this suite is asserting formatting/mapping behavior, mixingnull/undefinedin fixtures can hide regressions.
At minimum, the tests should be consistent about which representation is canonical for “no description”.
Summary of changes
Overview
This diff adds Sentry release tracking using tsup build-time injected metadata and includes some type/workflow alignment work around workouts and webhooks.
Key changes
-
Sentry
- Adds build-time globals
__HEVY_MCP_NAME__,__HEVY_MCP_VERSION__, and__HEVY_MCP_BUILD__insrc/index.ts. - Sets
Sentry.init({ release })usingprocess.env.SENTRY_RELEASE ??${name}@${version}``. - Updates the README to document that the release name is derived from the package version.
- Adds build-time globals
-
Build configuration (
tsup)- Reads and validates
package.jsonintsup.config.tsand injects constants viadefine(__HEVY_MCP_*).
- Reads and validates
-
Webhook tooling
- Treats webhook methods (
getWebhookSubscription,createWebhookSubscription,deleteWebhookSubscription) as optional on the client insrc/tools/webhooks.tsand tests. - Replaces missing generated webhook endpoints with explicit throwing stubs in
src/utils/hevyClientKubb.ts.
- Treats webhook methods (
-
Workout payload construction
- Refactors workout create/update request bodies to build a
workoutPayloadfirst and then wrap asPostWorkoutsRequestBody. - Updates workout tests to use ISO timestamps and adjusts one fixture’s
descriptionfromnulltoundefined.
- Refactors workout create/update request bodies to build a
| const isBuiltArtifact = | ||
| typeof __HEVY_MCP_BUILD__ === "boolean" ? __HEVY_MCP_BUILD__ : false; | ||
| if ( | ||
| isBuiltArtifact && | ||
| (typeof __HEVY_MCP_NAME__ !== "string" || | ||
| typeof __HEVY_MCP_VERSION__ !== "string") | ||
| ) { | ||
| throw new Error( | ||
| "Build-time variables __HEVY_MCP_NAME__ and __HEVY_MCP_VERSION__ must be defined.", | ||
| ); | ||
| } | ||
|
|
||
| const name = | ||
| typeof __HEVY_MCP_NAME__ === "string" ? __HEVY_MCP_NAME__ : "hevy-mcp"; | ||
| const version = | ||
| typeof __HEVY_MCP_VERSION__ === "string" ? __HEVY_MCP_VERSION__ : "dev"; | ||
|
|
||
| // Configure dotenvx with quiet mode to prevent stdout pollution in stdio mode | ||
| dotenvx.config({ quiet: true }); | ||
|
|
||
| // Sentry monitoring is baked into the built MCP server so usage and errors | ||
| // from users of the published package are captured for observability. | ||
| const sentryRelease = process.env.SENTRY_RELEASE ?? `${name}@${version}`; | ||
| const sentryConfig = { | ||
| dsn: "https://ce696d8333b507acbf5203eb877bce0f@o4508975499575296.ingest.de.sentry.io/4509049671647312", | ||
| release: sentryRelease, |
There was a problem hiding this comment.
The runtime guard checks that __HEVY_MCP_NAME__ / __HEVY_MCP_VERSION__ are strings, but the error message says they “must be defined”. That’s misleading while debugging (e.g., defined but wrong type, or empty string). Also, as written, a built artifact can still produce release = "hevy-mcp@dev" if __HEVY_MCP_VERSION__ is missing but __HEVY_MCP_BUILD__ is false (or not injected), which can silently degrade release tracking in production-like runs.
Suggestion
Tighten the invariant so built artifacts cannot silently fall back to dev, and make the thrown error accurately describe the condition.
For example:
const isBuiltArtifact = __HEVY_MCP_BUILD__ === true;
if (isBuiltArtifact) {
if (typeof __HEVY_MCP_NAME__ !== "string" || !__HEVY_MCP_NAME__) {
throw new Error(
"Invalid build metadata: __HEVY_MCP_NAME__ must be a non-empty string (tsup define).",
);
}
if (typeof __HEVY_MCP_VERSION__ !== "string" || !__HEVY_MCP_VERSION__) {
throw new Error(
"Invalid build metadata: __HEVY_MCP_VERSION__ must be a non-empty string (tsup define).",
);
}
}
const name = typeof __HEVY_MCP_NAME__ === "string" && __HEVY_MCP_NAME__ ? __HEVY_MCP_NAME__ : "hevy-mcp";
const version = typeof __HEVY_MCP_VERSION__ === "string" && __HEVY_MCP_VERSION__ ? __HEVY_MCP_VERSION__ : "dev";This keeps dev ergonomics while ensuring real builds fail fast if release metadata injection breaks.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
| getWebhookSubscription: async (): Promise<unknown> => { | ||
| throw new Error( | ||
| "Webhook subscription API not available. Please regenerate the client from the updated OpenAPI spec.", | ||
| ); | ||
| }, | ||
| createWebhookSubscription: async ( | ||
| _data: CreateWebhookSubscriptionRequest, | ||
| ): Promise<unknown> => { | ||
| throw new Error( | ||
| "Webhook subscription API not available. Please regenerate the client from the updated OpenAPI spec.", | ||
| ); | ||
| }, | ||
| deleteWebhookSubscription: async (): Promise<unknown> => { | ||
| throw new Error( | ||
| "Webhook subscription API not available. Please regenerate the client from the updated OpenAPI spec.", | ||
| ); | ||
| }, |
There was a problem hiding this comment.
You now have two competing strategies for unsupported webhook endpoints:
src/utils/hevyClientKubb.tsalways provides methods (throwing stubs)src/tools/webhooks.tstreats methods as optional and throws a different “not available” error when absent
In practice, for the default client the presence checks will always pass, so the “not available” branch is never used; for alternative injected clients, behavior diverges. This ambiguity increases test and runtime complexity and makes it unclear whether the API surface is optional or required.
Suggestion
Pick one strategy and apply it consistently:
Option A (recommended): make webhook methods non-optional in the HevyClient type used by the tools/tests and remove presence guards. Let the default client’s throwing stubs provide the single source of truth for the error.
Option B: remove the stubs from createClient() and keep methods truly optional + guards.
Given you already added clear stubs, Option A is simpler and avoids split behavior.
Reply with "@CharlieHelps yes please" if you want me to add a commit implementing Option A (remove optional methods + delete redundant guards) or Option B (remove stubs and keep guards).
# [1.17.0](v1.16.0...v1.17.0) (2025-12-20) ### Features * **sentry:** add release tracking ([#204](#204)) ([123b17c](123b17c))
2 participants
Adds Sentry release tracking by setting
releaseonSentry.init(defaults to build-time-injectedname@version, override viaSENTRY_RELEASE).Changes
Sentry.init({ release })insrc/index.tsusingSENTRY_RELEASE ?? \"${name}@${version}\", wherename/versionare injected at build time by tsup (define) with adevfallback for tests/unbundled runs.pnpm run check:typesby making the workout request-body construction compatible with generated types and by replacing webhook client calls that referenced missing generated endpoints with explicit stubs.Verification
src/tools/webhooks.ts:10-21keeps webhook methods typed as optional + presence guards even though the default client provides throwing stubs, to preserve a clear error message if a different client implementation is injected (tests rely on this) and until the OpenAPI client is regenerated.Closes #203.
✨ PR Description
Purpose: Add Sentry release tracking with build-time version injection to enable proper error correlation and deployment monitoring across published package versions.
Main changes:
anytype assertionsGenerated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how