feat: send structured logging messages to MCP clients - #510
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #510 +/- ##
==========================================
+ Coverage 90.80% 91.01% +0.20%
==========================================
Files 28 29 +1
Lines 1120 1168 +48
Branches 286 301 +15
==========================================
+ Hits 1017 1063 +46
Misses 46 46
- Partials 57 59 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
✨ PR Review
The PR cleanly introduces structured MCP client logging with good separation of concerns, fire-and-forget semantics, and solid test coverage. One functional bug stands out where the logging logic inadvertently captures all mutation failures as error-level MCP log messages, which was clearly not the intent.
2 issues detected:
🐞 Bug - `!shouldRetryRequest` is true for ALL non-GET methods, so every POST/PUT/DELETE failure unconditionally emits an error-level MCP log, flooding clients with spurious errors for expected application responses like 400 or 409.
Details: The "Hevy API request failed without retry" error-level log is emitted for every failed request where shouldRetryRequest returns false. Since shouldRetryRequest returns false for all non-GET methods (POST, PUT, DELETE) regardless of error type, every mutation failure — including expected 400, 409 (conflict), and 422 validation errors — generates an "error" level MCP log notification to the client. This will produce misleading noise for normal application-level errors on write operations.
File: src/utils/hevyClientKubb.ts (333-344)
🐞 Bug - `refreshReason` is evaluated in the outer handler scope before `getOrFetch` is invoked, so concurrent calls or in-flight fetches can cause the logged reason to be computed from stale or irrelevant cache state.
Details: refreshReason is computed from exerciseTemplateCatalogCache.size === 0 before getOrFetch is called. If two concurrent search-exercise-templates calls arrive when the cache is empty, both compute refreshReason = "initial-load". Only one actually triggers the fetch callback (the cache deduplicates), but both closures capture the same label. More critically, if the cache is non-empty but TTL-expired and a refresh is in-flight from a previous call, a new call will compute refreshReason = "ttl-expired" even though its callback will never execute since getOrFetch returns the in-progress fetch. The reason logged will therefore not reflect the actual trigger for the specific refresh event.
File: src/tools/templates.ts (286-303)
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
|
@Mergifyio refresh |
✅ Pull request refreshed |
Merge Queue Status
This pull request spent 10 minutes 7 seconds in the queue, with no time running CI. ReasonPull request #510 has been dequeued Queue conditions are not satisfied:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Requeued — the merge queue status continues in this comment ↓. |
|
I did not push a change because branch refresh and conflict resolution are outside this repair scope. Please resolve the conflict on — charlied/pr-check-repair |
|
Conflict repair is blocked. I re-fetched this branch and the current The branch is Please recreate or manually update the PR branch from current — charlied/pr-merge-conflict-repair |
|
Charlie could not resolve this conflict safely.
— charlied/pr-merge-conflict-repair |
|
@Mergifyio rebase |
☑️ Nothing to do, the required conditions are not metDetails
|
|
| Filename | Overview |
|---|---|
| src/utils/exercise-template-catalog.ts | Adds refresh reason reporting for exercise template catalog reloads, but warm-cache refreshes can still be reported as initial loads. |
| src/tools/templates.ts | Passes cache refresh events to structured logging and keeps logger failures isolated from tool behavior. |
| src/utils/hevyClientKubb.ts | Adds sanitized structured logging for Hevy API retries, rate limits, and terminal failures. |
| src/utils/mcp-client-logger.ts | Adds a fire-and-forget MCP client logger with connection checks and error isolation. |
| src/index.ts | Advertises MCP logging support and injects the client logger into API and template registration. |
Reviews (2): Last reviewed commit: "fix: resolve structured logging merge co..." | Re-trigger Greptile
| : exerciseTemplateCatalogCache.size === 0 | ||
| ? "initial-load" |
There was a problem hiding this comment.
Cache Refresh Reason Mislabels
When a warm cache entry expires or is invalidated by a concurrent refresh, size can be 0 before this call reaches getOrFetch. A concurrent refresh can then log initial-load even though the catalog was already loaded, which makes cache refresh telemetry report the wrong cause.
Unit Test Results 1 files 26 suites 1s ⏱️ Results for commit 2f79244. |
Merge Queue Status
This pull request spent 4 minutes 30 seconds in the queue, including 2 minutes 5 seconds running CI. Required conditions to merge
|
| : exerciseTemplateCatalogCache.size === 0 | ||
| ? "initial-load" | ||
| : "ttl-expired"; | ||
|
|
||
| return exerciseTemplateCatalogCache.getOrFetch( |
There was a problem hiding this comment.
Refresh reason still mislabels
This still chooses the refresh reason from exerciseTemplateCatalogCache.size before getOrFetch decides why it needs to fetch. If the catalog was already loaded but the entry expires or is cleared before this call reaches the cache, size is 0, so the log reports initial-load for a warm-cache refresh. That makes the MCP cache event report the wrong cause. The reason needs to come from the cache decision path, or otherwise distinguish a first load from an expired or removed existing entry.
Summary
Test plan
npm run checknpm run check:typesnpm run buildnpx vitest run --exclude tests/integration/**(21 files / 308 tests)npm run check:changesetgit diff --checkIncludes the non-empty minor changeset
.changeset/calm-coins-log.md.Resolves #499
✨ PR Description
Purpose: Implement structured logging infrastructure to send MCP client notifications for Hevy API request failures, retries, rate limits, and cache operations.
Main changes:
createMcpClientLoggerutility providing fire-and-forget logging to MCP clients with connection state validation and error isolationhevyClientKubbto emit structured logs for API failures, retries, rate limits with endpoint redaction and categorized severity levelsregisterTemplateToolswith optional logger parameter to emit structured logs on exercise template catalog refreshes with reason trackingGenerated 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