feat(doctor): add sentry doctor - #1501
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Design for a fast, read-only health check for existing Sentry installs: server-side truth via the Sentry API, local config capture across ecosystems, and an opt-in --fix path that reuses the existing sentry-wizard workflow in dry-run. Records two probe findings: dry-run is safe server-side (four write paths verified guarded), and `init --dry-run` spawns the user's dev server via an unguarded verifySetup call site — fixed as a prerequisite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…findings
- §5: fixHint -> remediation {human, agent}, since the terminal and --prompt
renderers want different text
- §7.8: captured config is untrusted input; allowlist before interpolating
- §10: default run writes nothing; --json/--report/upload are the three ways
to get the machine contract
- §11.1: default output mockups, --verbose and --report in the flag table
- §15: allowlist tests
- §18: PostHog review findings — two adoptions, one rejection, plus the
correction that its doctor lives in PostHog/wizard, not PostHog/posthog
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- §9: liveness is default because the reads that establish it create nothing. Key status via getProjectKeys().isActive catches revoked/rotated keys with no write; only egress/proxy/never-inits needs an event, so --live becomes the opt-in --send-test-event - §11: drop --offline (§14 already auto-degrades), --report (shell redirection), --verbose (--json serves it), --prompt (Fix block always prints). Agent detection now suppresses decoration per wizard-runner.ts:608 rather than switching render modes - §5: remediation collapses to one executable string — with one render, a second terser variant is the same instruction twice - §10: doctor writes no files at all; contract always carries every result - §18: reclassify PostHog's split remediation as considered-and-rejected, with the reason it is right for them and not for us Also fixes a stray code fence left in §5 by the previous commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sixteen tasks derived from the approved design spec, covering the capture/resolve/check/render pipeline, tier-1 through tier-3 checks, the opt-in live round-trip, the consent-gated support export, and --fix escalation to the setup workflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
checkGitStatus runs unconditionally and reaches execFileSync("git"), so
"spawns no child process" was false. Task 15 leans on this line.
…egex The wizard-runner-dry-run test asserted on handleFinalResult's source text, which passes if an extra ungated call is added and fails on harmless refactors. Replace it with a verifySetup spy asserting the property directly in both directions: not called under --dry-run, called on a dryRun:false success path. Also stubs verifySetup in the shared suite so success-path tests no longer fall through to the real implementation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Walk the project with a single collectGrep pass, classify matches against the init/build marker tables, redact secrets at the capture boundary, parse dependency manifests, and produce the Capture object that every downstream check reads from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds `resolveServerFacts()` which takes a local `Capture` and calls the Sentry API to produce `ServerFacts`. Every API call is wrapped in `tryFact` so a single failing endpoint leaves its field `undefined` (triggering `skip` in downstream checks) rather than crashing the run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tier-2 checks read Capture only (no API calls) and cover: init.present, config.dsn_set, config.environment, config.debug, config.sample_rate, build.upload_configured, and capture.complete. Auto-init platforms produce pass (not fail) when no explicit code call exists, and dynamic keys are treated as present-but-unknown. checks/index.ts combines TIER1_CHECKS and TIER2_CHECKS into the REGISTRY constant imported by the runner and reporter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the `sentry doctor` CLI command that runs the four-stage pipeline (capture, resolve, runChecks, render) and sets exit code 0/1 based on results. Registers `--send-test-event` and `--fix` flags with placeholder stubs for live.ts and fix.ts that later tasks will replace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replace the placeholder live.ts with the real implementation that sends a synthetic probe event to the Sentry ingest endpoint and optionally polls the issues search to confirm arrival. POST success is the primary signal (fail on network error); search-index lag is a warn, never a fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
After yielding the doctor report, offer to send it to Sentry support via Sentry.captureFeedback. Four gates prevent the prompt from appearing when inappropriate: no failures, non-TTY, agent-driven, or telemetry disabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replace the placeholder stub with a real implementation that runs `sentry init --dry-run` to produce a fix plan without mutating state. - Widen `runWizard` return type to `Promise<WorkflowRunResult | undefined>` - Add `codemodPlan` to `WizardOutput` type - Implement `deriveFeatures` to map failing checks to wizard features - Implement `runFix` that always passes `dryRun: true` to the wizard - Wizard failures are caught and warned, never rethrown Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
colorTag() produces semantic markup (<red>…</red>) that needs renderMarkdown() to become ANSI escape codes. Without it the tags render as literal text in the terminal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
maxMatchesPerFile: 1 stretches the 5000-result budget across more files instead of burning it on repeated matches in one SDK source file. Report export now sends a short feedback message with the full JSON as an attachment rather than stuffing 60KB into the message body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Issue title shows 'TestError' in Sentry, message carries the nonce. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Check ID tables, JSON output schema, exit codes, and agent usage guidance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When the DSN didn't resolve to a project, say so instead of the generic 'Sentry did not return X'. When the API call itself failed, say that too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keep judged keys in memory for checks, but omit them (and cwd) from the serialized report. Detect Swift trailing-closure inits, Android double-init, sentry.gg DSNs, and config files gitignore would hide. Send support export on every interactive yes, even with telemetry off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Those files were local planning artifacts, not repo convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
verifySetup is mocked to undefined on the success path, so the third argument is not "anything". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
caa147e to
189b1c4
Compare
Spring/Laravel configs used paren delimiters and never captured. JSON mode still prompted for support when stdin was a TTY. artifacts.uploaded now fails on stacks that upload debug files or source maps, and skips backend runtimes that do not. DSN scan I/O errors skip instead of reporting a missing DSN. Go/.NET keys and Java setters are read; android.double_init only counts Java/Kotlin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BYK
left a comment
BYK
left a comment
There was a problem hiding this comment.
I like the idea and I think the implementation is close to being ready. A lot of the things need to be extracted out from the CLI though, otherwise we'll keep piling information here and miss the opportunity to give control back to SDKs and make this proper once again.
Some stuff can live here, especially for better DSN detection but even some of that (including our existing code) should be moved out.
Finally, I think there are some missed opportunities to leverage the existing code in the CLI. This PR seems to reinvent a few wheels unnecessarily and is quite large to review and ship.
| Check whether Sentry is correctly set up and actually working | ||
|
|
||
| **Flags:** | ||
| - `--sendTestEvent - Send a synthetic event to the configured DSN and confirm it arrives (a write)` |
There was a problem hiding this comment.
I'd rename this to online or something to make it not sound like a subcommand.
|
|
||
| import type { Check } from "../types.js"; | ||
| import { TIER1_CHECKS } from "./tier1.js"; | ||
| import { TIER2_CHECKS } from "./tier2.js"; |
There was a problem hiding this comment.
These tiers need explanation. Why do we have 2 tiers? What do they stand for?
|
|
||
| /** Ecosystems that use a bundler/build plugin to upload symbolication data. */ | ||
| const UPLOAD_EXPECTING_ECOSYSTEMS = new Set([ | ||
| "javascript", |
There was a problem hiding this comment.
Ideally this list would come from a central place where all of our products can read from. Possibly the release registry?
| @@ -0,0 +1,313 @@ | |||
| /** | |||
| * One block scanner for every platform doctor understands. | |||
There was a problem hiding this comment.
This concept is a big no. We should not be rolling and maintaining our bonafide half-assed parser. We should be using something like treesitter or ast-grep or something.
| const SENTRY_PATTERN = /sentry/i; | ||
|
|
||
| /** Auto-init config files, even when gitignored. Not every xml/json. */ | ||
| const CONFIG_FILE_GLOBS = [ |
There was a problem hiding this comment.
I don't understand why we are doing this. If this is to find DSNs, our existing DSN discovery code should already cover this. If not, that code should be updated, not built upon here so the rest of the CLI can benefit.
|
|
||
| import type { ParsedManifest } from "./types.js"; | ||
|
|
||
| const MANIFEST_BASENAMES = |
There was a problem hiding this comment.
I like that this is only a 100-line file but again, all this meta information should live in an SDK-controlled central place. Probably the release registry. Then we can either dynamically download or compile that information or embed at build time. I prefer the dynamic version though.
| @@ -0,0 +1,180 @@ | |||
| /** | |||
| * Where Sentry gets configured, as data. | |||
There was a problem hiding this comment.
Again, CLI is not the place for this information
| @@ -0,0 +1,53 @@ | |||
| /** | |||
| * Redaction and untrusted-input validation for captured project files. | |||
There was a problem hiding this comment.
Don't understand why we need this but if we really do need it, it should be a generic helper at the output processing layer, not a command-specific exception.
| import { logger } from "../logger.js"; | ||
| import type { DoctorReport } from "./render.js"; | ||
|
|
||
| const FLUSH_TIMEOUT_MS = 3000; |
There was a problem hiding this comment.
Why do we need a custom flush timeout and why is it specific to here?
| * | ||
| * Self-hosted (SENTRY_URL set): only DSNs matching the configured | ||
| * host are valid. SaaS: only `*.sentry.io` DSNs are valid. | ||
| * host are valid. SaaS: `*.sentry.io` and `*.sentry.gg` DSNs are valid. |
| id: "project.environments", | ||
| status: "pass", | ||
| detail: `${environments.length} environment(s): ${environments.join(", ")}.`, | ||
| }; | ||
| }, | ||
| }; | ||
|
|
||
| const releaseAttribution: Check = { | ||
| id: "release.attribution", | ||
| run: (ctx) => { |
There was a problem hiding this comment.
Bug: The hasUploadedArtifacts function incorrectly returns false if any artifact check is empty, even if other checks failed, leading to incorrect "no artifacts" reports.
Severity: MEDIUM
Suggested Fix
The logic should be adjusted to only return false when all three checks (dsyms, bundles, maps) have successfully completed and all are false. If any check returns true, the function should return true. If any check returns undefined and none are true, the function should return undefined to signify an incomplete check.
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/cli/src/lib/doctor/checks/tier1.ts#L303-L312
Potential issue: The `hasUploadedArtifacts` function checks for dsyms, bundles, and
source maps in parallel. Its logic for combining the results is flawed. If one check
successfully returns an empty list (`false`) while another check fails due to a network
error (`undefined`), the function incorrectly returns `false` (no artifacts found). This
causes the `sentry doctor` command to report a false-positive failure, telling the user
that no debug files exist when, in fact, the check was incomplete due to a transient
error. The correct behavior would be to return `undefined` to indicate the check's
outcome is unknown.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3a9f554. Configure here.
| if (dsyms === false || bundles === false || maps === false) { | ||
| return false; | ||
| } | ||
| return; |
There was a problem hiding this comment.
Artifact lookup can abort server facts
Medium Severity
hasUploadedArtifacts now calls resolveOrgRegion outside tryFact, and populateEndpointFacts awaits it unguarded in Promise.all. An AuthError or HostScopeError from region resolution rejects the whole batch, so keys, issues, environments, and releases are dropped and resolveServerFacts can throw instead of returning partial facts.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3a9f554. Configure here.
|
@BYK sorry, this is definitely far from being ready for review (should've made it a draft), and I haven't had even a slightest look at the implementation myself 🙈 Daniel just asked to open a PR so he could take a look and I just did that. I like the idea of having SDK-specific bits in the registry, so I'll look into that (and all of your other comments) after I'm back from sabbatical. Tree-sitter is also something I'd love to use here (I thought the agent had opted for it, but apparently it hadn't). |
|
@romtsn no worries! Looking forward for the updates! |
2 participants
Summary
Adds
sentry doctor: a read-only health check of an existing Sentry install. It scans the project, asks the API, and reports what is configured, what is broken, and what to fix.--send-test-eventdoes a live round-trip.--fixis a dry-run setup plan, never a write.cwd. Interactive runs can send the report to support on an explicit yes, even with telemetry off.project.pbxprojparsing (Xcode run scripts stay invisible; Fastfile only).Test plan
pnpm --filter sentry exec vitest run test/lib/doctor test/commands/doctor.test.ts test/lib/dsn/code-scanner.test.ts— 176 testspnpm --filter sentry exec tsc --noEmit -p tsconfig.jsonsentry doctor --jsonon~/Workspace/android,~/Workspace/empower,~/Workspace/ios🤖 Generated with Claude Code