feat: report fatal CLI/MCP errors to Sentry with PICKLAB_TELEMETRY opt-out#11
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e5ba3b8c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| for (const exception of event.exception?.values ?? []) { | ||
| if (exception.value !== undefined) { | ||
| exception.value = redactSecrets(exception.value); |
There was a problem hiding this comment.
When a fatal error is an operational command failure, the exception message already contains command arguments and device output; for example packages/android/src/adb.ts:293 builds messages like adb -s ... install -r <apkPath> plus stderr/stdout. This hook only runs redactSecrets, so non-secret APK paths, serials, package/activity names, and adb/log output are still sent to Sentry, contradicting the new privacy contract that no commands, arguments, or device data are collected. Sanitize or replace these exception values before returning the event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Triaged as docs-honesty rather than data-stripping: the failing command's output is the primary debugging signal for an automation CLI, so we keep it (run through the repo's redactSecrets) and instead corrected the README/INSTALL copy in 9a392cb to say exactly that — messages can reference the failing command and its output, secrets redacted. Combined with the payload hardening in the sibling fix (breadcrumbs dropped, server_name/modules deleted, ContextLines/local variables off), the docs now match the payload precisely.
| } | ||
| const require = createRequire(import.meta.url); | ||
| const { version } = require("../package.json") as { version: string }; | ||
| Sentry.init({ |
There was a problem hiding this comment.
With the bare Sentry.init call here, the Node SDK keeps its default integrations; Sentry's own integrations table documents contextLinesIntegration as auto-enabled and adding source code to stack frames, and localVariablesIntegration as default for adding local variables (see the Node integrations docs). In fatal CLI paths this can attach source snippets and in-scope values such as parsed args/options even if the error message is sanitized, violating the new README/INSTALL promise that no file contents, commands, or arguments are collected. Filter these integrations out or explicitly turn stack-frame context/variables off.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9a392cb: includeLocalVariables: false and ContextLines filtered out of the default integrations. (localVariablesIntegration is opt-in via includeLocalVariables, but it's now explicitly off as a belt-and-braces measure.)
| import { captureFatal, initTelemetry } from "./telemetry.js"; | ||
|
|
||
| process.exitCode = await runMcpServe(); | ||
| initTelemetry(); |
There was a problem hiding this comment.
Because ESM static imports are evaluated before the module body, any startup failure while loading ./commands/mcp.js or its MCP-server dependencies happens before this initTelemetry() call and before the surrounding try/catch exists. That leaves dependency/load-time startup failures unreported despite this change aiming to capture fatal MCP startup errors; move the server import behind telemetry initialization with a dynamic import or a tiny bootstrap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9a392cb: both bins now dynamic-import their heavy entry module after initTelemetry() and inside the try block, so module-load-time failures are captured. Side effect worth noting: the no-sudo bundle scanner only followed static import specifiers, so it was extended to trace dynamic import("...") chunks — coverage of the MCP chunk graph is restored (verified by the control test failing before the scanner fix).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a392cbb52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| release: `picklab@${version}`, | ||
| tracesSampleRate: 0, | ||
| includeLocalVariables: false, | ||
| integrations: (defaults) => defaults.filter((i) => i.name !== "ContextLines"), |
There was a problem hiding this comment.
When telemetry is enabled for a fatal CLI/MCP error, this keeps every Sentry default integration except ContextLines. The Node SDK's default nodeContextIntegration adds device/environment context, and device names are typically host names, while the new telemetry contract says only message/stack plus OS/app version are sent and that hostnames are stripped; deleting event.server_name later does not remove event.contexts.device.name. Please disable the default context integration(s) or scrub event.contexts before sending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eeadb80 by closing the whole family: defaultIntegrations: false with an explicit allowlist (inboundFilters, functionToString, linkedErrors, dedupe, onUncaughtException, onUnhandledRejection, nodeContext), and beforeSend prunes event.contexts to exactly os + runtime. Verified with a stub transport: context keys are ["runtime","os"], no device/culture/app.
| release: `picklab@${version}`, | ||
| tracesSampleRate: 0, | ||
| includeLocalVariables: false, | ||
| integrations: (defaults) => defaults.filter((i) => i.name !== "ContextLines"), |
There was a problem hiding this comment.
With telemetry enabled, this keeps Node's default captureConsoleIntegration, so normal console.error calls become Sentry events. The MCP server logs picklab mcp server: listening on stdio during successful startup and then remains alive long enough to send that non-fatal event, even though the README/INSTALL text says only fatal errors are collected. Please filter out CaptureConsole (or disable defaults) so only captureFatal reports events.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Structurally impossible as of eeadb80: defaultIntegrations: false + explicit allowlist, and captureConsoleIntegration is not in it (it also isn't a default integration in @sentry/node v10 — console capture only ships as breadcrumbs, which beforeBreadcrumb drops). The MCP stderr banner can never become an event.
| beforeSend: (event) => { | ||
| delete event.server_name; | ||
| delete event.modules; | ||
| if (typeof event.message === "string") { | ||
| event.message = redactSecrets(event.message); | ||
| } | ||
| for (const exception of event.exception?.values ?? []) { | ||
| if (exception.value !== undefined) { | ||
| exception.value = redactSecrets(exception.value); | ||
| } | ||
| } | ||
| return event; |
There was a problem hiding this comment.
For fatal errors that carry custom properties, this hook redacts only event.message and exception.value, but the kept default extraErrorDataIntegration can attach non-native error fields under event.extra. For example, CommandError has an enumerable result containing command stdout/stderr, so a failed command that prints a token in its output can still send that raw output to Sentry despite the new telemetry contract saying command output is redacted and only message/stack plus OS/app version are collected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Moot as of eeadb80: extraErrorDataIntegration is not in the explicit allowlist (nor is it a default integration in v10), so custom error properties are never attached. The only payload is message + stack (redacted), os/runtime contexts, and release.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeadb80233
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Sentry.linkedErrorsIntegration(), | ||
| Sentry.dedupeIntegration(), | ||
| Sentry.onUncaughtExceptionIntegration(), | ||
| Sentry.onUnhandledRejectionIntegration(), |
There was a problem hiding this comment.
With the default onUnhandledRejectionIntegration() options, Sentry uses mode: "warn"; installing that unhandledRejection listener changes Node 20's default behavior from throwing/exiting to warning and continuing. If any background promise in picklab mcp serve or a CLI command rejects outside the awaited top-level path, the process can keep running after a fatal error instead of failing the command/server. Configure this integration with mode: "strict" (or omit it and rely on the explicit top-level catches) to preserve crash semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 57e639e: onUnhandledRejectionIntegration({ mode: "strict" }) — verified behaviorally with a scratch script: an unhandled rejection exits 1 and never reaches a pending timer, preserving Node 20 crash semantics.
| npx --yes @sentry/cli sourcemaps inject packages/cli/dist | ||
| npx --yes @sentry/cli sourcemaps upload --release "picklab@$VERSION" packages/cli/dist |
There was a problem hiding this comment.
When SENTRY_AUTH_TOKEN is present but invalid, lacks upload permissions, or Sentry is temporarily unavailable, either sentry-cli command exits non-zero; GitHub Actions treats non-zero run-step exits as a failed step, so this tag workflow stops before the following npm publish and GitHub release steps even though source-map upload is ancillary (and the missing-token path already skips). Make this upload soft-fail or move it after publishing so telemetry infrastructure cannot block a release.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 57e639e: the inject+upload block now warns (::warning::) and exits 0 on sentry-cli failure, so an invalid token or Sentry outage can never block npm publish. The empty-token skip is unchanged.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Adds Sentry error reporting to the published CLI package, on by default with
PICKLAB_TELEMETRY=0opt-out.Refs #10 — implements the SDK integration slice; the remaining checklist item (setting the
SENTRY_AUTH_TOKENrepo secret) is tracked on the issue.What changed
@sentry/nodeinitialized in both bins (picklab,picklab-mcp); fatal errors captured and flushed in the top-level catches (the MCP bin previously had none — startup failures were unhandled rejections).server_name/modulesstripped, exception messages run through the repo'sredactSecrets;tracesSampleRate: 0, no PII.SENTRY_AUTH_TOKENsecret is absent).Tested
bun run typecheck,bun run test(556 passed, incl. 8 telemetry gate tests),bun run build.PICKLAB_TELEMETRY=0— commands behave identically, Sentry never initializes.Not tested