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.
|
|
||
| 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?
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