Conversation
mydea
added this pull request to stack #24498
September 18, 2026 10:39
Contributor
size-limit report 📦
|
mydea
force-pushed
the
feat/hono-orchestrion-instrumentation
branch
from
September 21, 2026 07:46
1e729ec to
74f5d86
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if (!IS_DEV) { | ||
| expect(attrValue(serverSpan, 'sentry.segment.name.source')).toBe('route'); | ||
| expect(attrValue(serverSpan, 'http.route')).toBe('/manual-route'); | ||
| } |
There was a problem hiding this comment.
Conditional assertions in one test
Low Severity
This was flagged because the testing conventions ask to split conditionals in a single test into separate tests per path. Both Mastra e2e tests branch on IS_DEV inside one case, so the route-enriched prod assertions and the un-enriched dev assertions never run as independent tests.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 74f5d86. Configure here.
mydea
force-pushed
the
feat/hono-orchestrion-instrumentation
branch
from
September 21, 2026 08:13
d3071cb to
bef613e
Compare
| // A Hono `matchResult[0]` entry: `[[handler, routeMeta], paramIndexMap]`. `compose` reads the handler | ||
| // at `entry[0][0]`; the `matchedRoutes` getter reads `routeMeta` at `entry[0][1]`. | ||
| // oxlint-disable-next-line typescript/no-explicit-any | ||
| type MatchedHandlerEntry = [[any, any], any]; |
There was a problem hiding this comment.
Unguarded any in Hono integration
Low Severity
New SDK source uses any (and oxlint suppressions) on MatchedHandlerEntry, channel arguments, and tracingChannel payloads without a comment explaining why a safer type is not possible. This was flagged because the review rules require that explanation on each new any in production code.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit bef613e. Configure here.
Add `honoIntegration`, the auto-instrumentation that hooks Hono through the orchestrion module transform (`node:diagnostics_channel`) so requests are route-enriched without a manual `sentry()` middleware, plus its manual counterpart `honoMiddleware`. Register it in `getTracingIntegrations()` and re-export both from the server runtimes (node, cloudflare, bun, deno, and the serverless/meta-framework packages). Adds the orchestrion transform config for `hono`, node-integration-tests for the auto-instrumentation, and a new orchestrion-based `hono-4` e2e app (the middleware-based app now lives as `hono-4-legacy`). node-mastra now asserts route-enriched Hono spans in prod, where Hono is external and instrumented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An internal app.request() runs in a new Hono context but the same isolation scope, so the request-handling dedup short-circuited the inner Sentry middleware before it could capture context.error. When an outer handler swallowed a failed internal response (degrading to a 200), the inner route's error was never reported. The deduplicated middleware now still captures its own context's error, without re-naming the transaction or overwriting request data. Also streamline the orchestrion hono config comments, add a named-function middleware span test, and add node-integration coverage for the inner-error case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The auto-instrumentation injection loop used a raw arity check to decide which matched handlers to wrap as middleware spans. Use the shared isMiddleware helper instead, which unwraps onError-composed sub-app handlers before checking arity — a case the inline check missed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Arity alone cannot tell a middleware from a route handler declared with an unused `next` param. The matched entries carry their registration routeMeta, so apply the same positional heuristic as wrapSubAppMiddleware: within a method+path group the last handler is the route handler and earlier ones are middleware; `.use()` (method 'ALL') falls back to arity. Adds node-integration coverage for the arity-2 route handler case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
force-pushed
the
feat/hono-orchestrion-instrumentation
branch
from
September 21, 2026 09:10
bef613e to
8941894
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8941894. Configure here.
| expect(attrValue(serverSpan!, 'sentry.segment.name.source')).toBe('route'); | ||
| expect(attrValue(serverSpan!, 'http.route')).toMatch(/^\/api\/agents\/:[^/]+\/generate$/); | ||
| expect(serverSpan!.name).toMatch(/^POST \/api\/agents\/:[^/]+\/generate$/); | ||
| } |
There was a problem hiding this comment.
Conditional assertions in Mastra tests
Low Severity
These tests branch on IS_DEV inside a single case instead of splitting prod and dev. The dev path also never asserts that http.route and sentry.segment.name.source stay absent, so the un-instrumented Hono behavior is not actually locked in. This is flagged because the review rules require splitting conditional tests and asserting omitted payload fields.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 8941894. Configure here.
s1gr1d
reviewed
Sep 21, 2026
Comment on lines
+108
to
+121
| /** | ||
| * Per-request Context hook: the heart of the automatic instrumentation. | ||
| * | ||
| * `#dispatch` builds `new Context(req, { matchResult })` before its single-handler fast-path check, | ||
| * passing the live `matchResult` array. We: | ||
| * 1. wrap the already-matched MIDDLEWARE handlers (arity ≥ 2) for spans — route handlers (arity < 2) | ||
| * are covered by the request span and left as-is; | ||
| * 2. prepend the Sentry request/response middleware, so it runs first in the composed chain. That | ||
| * both drives route naming / request data / error capture (from inside the chain, with the | ||
| * Context) and forces the ≥2-handler `compose` path, so there is no fast-path gap. | ||
| * | ||
| * All of this runs per request, so it works on Cloudflare (no module-scope publish) and needs no | ||
| * app-instance patching or app-construction hook. | ||
| */ |
Member
There was a problem hiding this comment.
This comment is not up-to-date with the "detection" code anymore.
Member
Author
There was a problem hiding this comment.
I updated comments generally!
|
|
||
| const effectiveShouldHandleError = | ||
| (scope[HONO_SHOULD_HANDLE_ERROR] as SentryHonoMiddlewareOptions['shouldHandleError']) ?? shouldHandleError; | ||
| responseHandler(context, effectiveShouldHandleError); |
Member
There was a problem hiding this comment.
Should we maybe clear the flags (HONO_REQUEST_HANDLED and HONO_SHOULD_HANDLE_ERROR) here after the response?
Member
Author
There was a problem hiding this comment.
this should not be necessary, because:
- When there's a per-request isolation scope, getRequestScope stores them there. The SDK forks a fresh isolation scope per request, so the object (and its flags) is discarded when the request ends.
- When there isn't one (isolationScope === getDefaultIsolationScope()), it deliberately falls back to the Hono Context, not the default scope — and Hono builds a new Context per dispatch. Also per-request.
Restores coverage lost when the shared Hono unit tests were removed during the move to @sentry/server-utils. The observable route-name and middleware span-status behaviors move into the node-integration `hono` suite; the route-hook's non-invasive prototype patching (invisible to spans/events) is restored as a focused unit test. - resolveRouteName: overlapping catch-all handler and middleware-only path fallback - wrapMiddlewareWithSpan: 3xx/4xx middleware errors do not set an error span status, 5xx does - installRouteHookOnPrototype: preserves name/length/symbols/prototype of the original route method Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The dedup branch of createHonoRequestMiddleware calls captureContextError, but the mock only stubbed requestHandler/responseHandler, so the deduplication tests threw "No captureContextError export is defined on the mock". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Trim the comments in honoIntegration to the non-obvious reasoning (Hono's internal matchResult shape, the cached-array injection guard, the synchronous Context-constructor timing, the compose fast-path) and drop the narration of what the code already shows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
marked this pull request as ready for review
September 21, 2026 12:52
mydea
requested review from
JPeer264,
isaacs,
nicohrubec and
s1gr1d
and removed request for
a team
September 21, 2026 12:52
The Hono auto-instrumentation added to the Node SDK defaults pushes the ESM bundle to 135.3 KB, just over the 135 KB limit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+85
to
+88
| // outer context never sees the error. Route naming and request data stay owned by the request | ||
| // that ran first, so only the error is captured here. | ||
| const dedupShouldHandleError = | ||
| (scope[HONO_SHOULD_HANDLE_ERROR] as SentryHonoMiddlewareOptions['shouldHandleError']) ?? shouldHandleError; |
Contributor
There was a problem hiding this comment.
Bug: When both automatic honoIntegration and manual honoMiddleware are used, an error can be captured twice, leading to duplicate reports if dedupeIntegration is disabled.
Severity: LOW
Suggested Fix
To prevent duplicate error capturing, the logic should be updated. One approach is to check if the error has already been captured before attempting to capture it again. For example, the deduplicated middleware branch could skip calling captureContextError, or the responseHandler in the auto-middleware could add a check to see if the error was already handled by a downstream manual middleware.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/server-utils/src/integrations/hono/createHonoMiddleware.ts#L85-L88
Potential issue: When a user enables both the automatic `honoIntegration` and manually
adds the `honoMiddleware`, an error in a route handler can be captured twice. The
auto-instrumented middleware runs first, sets a `HONO_REQUEST_HANDLED` flag, and
proceeds. The manual middleware then executes, sees the flag, and captures the error in
its deduplication branch. After the handler chain completes, the original
auto-instrumented middleware's `responseHandler` also runs and captures the same error
again. This results in duplicate error reports for users who have disabled the default
`dedupeIntegration`.
Did we get this right? 👍 / 👎 to inform future reviews.
This branch has not been deployed
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
2 participants
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.
Second of two stacked PRs splitting the Hono instrumentation rework (originally #24371). Stacked on #24496 — review/merge that first; the diff here is against the base PR's branch.
Adds
honoIntegration, the auto-instrumentation that hooks Hono through the orchestrion module transform (node:diagnostics_channel) so requests are route-enriched without a manualsentry()middleware, plus its manual counterparthonoMiddleware. Registers it ingetTracingIntegrations()and re-exports both from the server runtimes (node, cloudflare, bun, deno, and the serverless / meta-framework packages).Also adds the orchestrion transform config for
hono, node-integration-tests for the auto-instrumentation, and a new orchestrion-basedhono-4e2e app (the middleware-based app now lives ashono-4-legacy, added in the base PR).node-mastranow asserts route-enriched Hono spans in prod, where Hono is external and orchestrion-instrumented.🤖 Generated with Claude Code