fix(sentry): restore guards lost in the main reset and quiet expected poll failures - #1311
fix(sentry): restore guards lost in the main reset and quiet expected poll failures#1311pedramamini wants to merge 2 commits into
Conversation
… poll failures `main` was reset after the accidental rc promotion, which took PR #1213 and PR #1246 with it - their merge commits are no longer reachable, so three already-fixed field issues went live again on 0.17.3. Restores both byte-for-byte so the eventual rc->main merge converges, then addresses the remaining stable-channel noise. Restored (regressed off main): - MAESTRO-1G (29 events/14d): `shouldDropSentryEvent` only matched the libuv fs spelling `EPIPE: broken pipe`. Node emits a bare `write EPIPE` for stream and socket writes to a dead pipe, so the rule never fired. - MAESTRO-2S / MAESTRO-2Z: `stats:record-session-created` and `-closed` threw "Database not initialized" when an agent was restored on boot ahead of the stats DB. Both callers are fire-and-forget and ignore the return. - MAESTRO-V5: `sessions:setMany` / `setAll` EACCES on maestro-sessions.json. - MAESTRO-KA: `isExpectedGroomingFailure()` for the group-chat groom path, including the follow-up that scopes the regex to `Agent ... is not available`. New: - MAESTRO-JS (53 occ) and MAESTRO-K7: extend `isExpectedGroomingFailure()` to cover a groom process that would not launch and revoked provider credentials. Both are user-environment conditions that `resetContext` already recovers from by starting a fresh session. Only the group-chat call site suppresses; the user-initiated groom paths in context.ts and director-notes.ts still surface. - MAESTRO-RR (478 occ, the noisiest app-code issue on stable): the codex quota sampler carved out 401/403 but still reported `http 503`. The sampler runs on a timer, so one ChatGPT outage reports once per tick per install. Skip 408/429/5xx too; a 4xx that implies we sent a malformed request still reports. - MAESTRO-KE (70 occ): `isGitHubConnectivityError` missed GitHub-side 5xx, so `HTTP 504: Gateway Timeout` paged Sentry on every Cue poll tick during a GitHub outage. The poller already retries on its own schedule. - MAESTRO-W9: the getGlobalStats incremental loops report EACCES on transcripts under ~/.claude that the agent CLI owns and we merely discovered. Extends the existing RangeError carve-out to expected fs error codes. Adds 27 regression tests, each verified to fail against the unfixed source.
📝 WalkthroughWalkthroughThe PR expands expected-error classification across Codex usage sampling, GitHub polling, session parsing, grooming, stats initialization, and shared Sentry filters, with tests covering suppressed and reportable error paths. ChangesExpected upstream failures
IPC error handling
Startup and shared filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryRestores readiness and expected-error guards while reducing Sentry noise from recoverable environment and upstream failures.
Confidence Score: 5/5The PR appears safe to merge, with the new guards preserving existing recovery behavior and focused tests covering the expanded classifications. The changes primarily narrow telemetry reporting for explicitly recoverable conditions, while unexpected errors continue through existing reporting paths and operational retries or fallbacks remain intact. Important Files Changed
Reviews (1): Last reviewed commit: "fix(sentry): restore guards lost in the ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR restores and extends Sentry-noise guards that were lost during a main reset, and adds additional filtering for known expected, user-environment failures so stable-channel crash triage reflects actionable defects.
Changes:
- Expand Sentry drop filters for common broken-pipe (
write/read EPIPE) and IPC persistence permission (EACCES) noise. - Guard stats IPC “record session created/closed” writes during early startup when the stats DB is not ready.
- Suppress expected operational failures from group-chat grooming, Codex usage sampling, GitHub Cue polling, and global transcript scans, with targeted regression tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/shared/sentryFilters.ts | Broaden Sentry noise filtering for EPIPE variants and IPC write EACCES cases. |
| src/main/ipc/handlers/stats.ts | Skip fire-and-forget lifecycle stats writes when the stats DB is not initialized yet. |
| src/main/ipc/handlers/groupChat.ts | Centralize “expected grooming failures” predicate and use it to avoid Sentry reporting in resetContext recovery. |
| src/main/ipc/handlers/agentSessions.ts | Treat expected filesystem read failures during transcript scans as non-Sentry noise while keeping local warnings. |
| src/main/cue/cue-github-poller.ts | Treat GitHub-side HTTP 5xx as connectivity noise for Cue polling. |
| src/main/agents/codex-usage-sampler.ts | Treat throttling/degraded-upstream HTTP statuses as expected for periodic quota sampling. |
| src/tests/shared/sentryFilters.test.ts | Add regression tests for EPIPE stream spelling and sessions:setMany EACCES filtering. |
| src/tests/main/ipc/handlers/stats.test.ts | Add coverage for stats lifecycle handlers skipping when DB is not ready. |
| src/tests/main/ipc/handlers/groupChat.test.ts | Add coverage for the expanded expected grooming failure predicate. |
| src/tests/main/ipc/handlers/agentSessions.test.ts | Add coverage for expected transcript read error classification. |
| src/tests/main/cue/cue-github-poller.test.ts | Add coverage ensuring GitHub 5xx does not report to Sentry during polling. |
| src/tests/main/agents/codex-usage-sampler.test.ts | Update expectations for which HTTP statuses should and should not trigger Sentry reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Windows. These are environmental, never a Maestro bug, so we keep the local | ||
| * warn but skip Sentry to avoid telemetry noise (MAESTRO-W9). Mirrors the | ||
| * EXPECTED_FS_ERROR_CODES carve-out in shared-history-manager.ts. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/__tests__/main/ipc/handlers/stats.test.ts (1)
530-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the no-broadcast contract in both startup guard tests.
Both tests verify the return value and skipped database write, but a regression that emits
stats:updatedbefore returning would still pass.
src/__tests__/main/ipc/handlers/stats.test.ts#L530-L543: assert that the session-created path does not broadcast.src/__tests__/main/ipc/handlers/stats.test.ts#L560-L568: assert that the session-closed path does not broadcast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/main/ipc/handlers/stats.test.ts` around lines 530 - 543, Update both startup guard tests in src/__tests__/main/ipc/handlers/stats.test.ts (lines 530-543 and 560-568) to assert that the stats:updated broadcast is not emitted when the stats database is uninitialized. Add the no-broadcast assertion to the session-created test and the session-closed test while preserving their existing return-value and skipped-write checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/main/cue/cue-github-poller.test.ts`:
- Around line 983-989: Add an assertion after advancing timers and before
inspecting Sentry calls that verifies mockExecFile received the expected `pr
list` invocation, confirming the injected failure path executed. Keep the
existing zero-cue:github:doPoll Sentry assertion unchanged.
In `@src/main/cue/cue-github-poller.ts`:
- Around line 100-104: Update resolveRepo() so failures from gh repo view that
represent GitHub connectivity outages or rate limits are re-thrown instead of
converted to null. This must allow doPoll() to pass them through
isGitHubConnectivityError so the outer handler suppresses expected GitHub 5xx
and rate-limit failures, while preserving null for non-connectivity
repository-discovery failures.
---
Nitpick comments:
In `@src/__tests__/main/ipc/handlers/stats.test.ts`:
- Around line 530-543: Update both startup guard tests in
src/__tests__/main/ipc/handlers/stats.test.ts (lines 530-543 and 560-568) to
assert that the stats:updated broadcast is not emitted when the stats database
is uninitialized. Add the no-broadcast assertion to the session-created test and
the session-closed test while preserving their existing return-value and
skipped-write checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22110f74-7cba-411d-bca2-dc6d86015890
📒 Files selected for processing (12)
src/__tests__/main/agents/codex-usage-sampler.test.tssrc/__tests__/main/cue/cue-github-poller.test.tssrc/__tests__/main/ipc/handlers/agentSessions.test.tssrc/__tests__/main/ipc/handlers/groupChat.test.tssrc/__tests__/main/ipc/handlers/stats.test.tssrc/__tests__/shared/sentryFilters.test.tssrc/main/agents/codex-usage-sampler.tssrc/main/cue/cue-github-poller.tssrc/main/ipc/handlers/agentSessions.tssrc/main/ipc/handlers/groupChat.tssrc/main/ipc/handlers/stats.tssrc/shared/sentryFilters.ts
| * | ||
| * A 5xx from api.github.com counts: `HTTP 504: 504 Gateway Timeout` and friends | ||
| * mean GitHub itself is degraded, which is the same "can't reach the API right | ||
| * now" condition as a dropped socket. The poller retries on its own schedule, so | ||
| * paging Sentry on every tick of a GitHub outage is pure noise (MAESTRO-KE). |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Route repository discovery 5xx failures through this classifier.
When config.repo is unset, resolveRepo() captures a gh repo view 5xx and returns null, so doPoll() never reaches isGitHubConnectivityError. Re-throw connectivity failures with rate limits so the outer handler suppresses expected GitHub outages.
Proposed fix
- if (isGitHubRateLimitError(err)) {
+ if (isGitHubRateLimitError(err) || isGitHubConnectivityError(err)) {
throw err;
}Also applies to: 127-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/cue/cue-github-poller.ts` around lines 100 - 104, Update
resolveRepo() so failures from gh repo view that represent GitHub connectivity
outages or rate limits are re-thrown instead of converted to null. This must
allow doPoll() to pass them through isGitHubConnectivityError so the outer
handler suppresses expected GitHub 5xx and rate-limit failures, while preserving
null for non-connectivity repository-discovery failures.
Addresses PR review findings on #1311. - `resolveRepo()` runs before the poll and has its own `captureException`, so a `gh repo view` 5xx during a GitHub outage still paged Sentry once per tick for every auto-detect trigger - the doPoll-side fix alone left MAESTRO-KE half covered. Suppresses connectivity failures there too. Diverges from the suggested "re-throw alongside rate limits": routing these into doPoll's catch would newly drag them through the first-poll seed-marker path, which changes event-loss semantics for a telemetry fix. Skipping the Sentry call keeps the existing skip-the-poll behavior byte for byte. - Assert the injected `pr list` failure actually executed in the 5xx test - a zero-Sentry assertion would also pass if polling bailed out earlier. - Drop the claim that the new fs carve-out mirrors EXPECTED_FS_ERROR_CODES in shared-history-manager.ts. That helper is on `rc`, not `main`.
|
All three review findings were valid and are addressed in b0b3587. CodeRabbit, I did not take the proposed CodeRabbit, Copilot, |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/main/cue/cue-github-poller.test.ts`:
- Around line 999-1017: Add an assertion in the test case around
createCueGitHubPoller to verify the mocked gh repo view command was invoked,
before inspecting Sentry calls. Keep the existing 503 mock and suppression
assertion unchanged, ensuring the test proves repository auto-detection
executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fea88d8-5b03-4b05-93d4-08939f624481
📒 Files selected for processing (3)
src/__tests__/main/cue/cue-github-poller.test.tssrc/main/cue/cue-github-poller.tssrc/main/ipc/handlers/agentSessions.ts
| it('does not report a GitHub-side 5xx during repo auto-detection (MAESTRO-KE)', async () => { | ||
| // Repo auto-detection runs before the poll, so a `gh repo view` failure | ||
| // short-circuits doPoll entirely and has to suppress on its own. | ||
| const config = makeConfig({ repo: undefined }); | ||
| mockExecFile.mockImplementation((_c, args, _o, cb) => { | ||
| if ((args as string[]).includes('--version')) return cb(null, '2.0.0', ''); | ||
| return cb(new Error('HTTP 503: 503 Service Unavailable (https://api.github.com)'), '', ''); | ||
| }); | ||
|
|
||
| const cleanup = createCueGitHubPoller(config); | ||
| await vi.advanceTimersByTimeAsync(2100); | ||
|
|
||
| const sentryCalls = mockCaptureException.mock.calls.filter( | ||
| (c) => (c[1] as { operation: string }).operation === 'cue:github:resolveRepo' | ||
| ); | ||
| expect(sentryCalls).toHaveLength(0); | ||
|
|
||
| cleanup(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prove that repository auto-detection executed.
The test only checks that no cue:github:resolveRepo Sentry event was captured. If polling exits before gh repo view, the assertion still passes without exercising the injected 503 path. Add an invocation assertion before inspecting Sentry calls.
Proposed fix
const cleanup = createCueGitHubPoller(config);
await vi.advanceTimersByTimeAsync(2100);
+ expect(
+ mockExecFile.mock.calls.some((c) => {
+ const args = c[1] as string[];
+ return args.includes('repo') && args.includes('view');
+ })
+ ).toBe(true);
+
const sentryCalls = mockCaptureException.mock.calls.filter(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/main/cue/cue-github-poller.test.ts` around lines 999 - 1017,
Add an assertion in the test case around createCueGitHubPoller to verify the
mocked gh repo view command was invoked, before inspecting Sentry calls. Keep
the existing 503 mock and suppression assertion unchanged, ensuring the test
proves repository auto-detection executed.
|
CI note: the
The last three commits on The failures look environmental rather than logical: every one is a ~1.1s Flagging rather than fixing, since it is unrelated to this PR's scope. Worth its own issue. Local validation for the changes here: 207/207 across the six affected suites, |
Field-crash triage against the
channel:stable(main) population on release 0.17.3.The headline finding: two merged PRs are no longer on
mainmainwas reset after the accidental rc promotion (see theci(guard): block the rc line from reaching maincommits). That reset also took two legitimately-merged main-line PRs with it:origin/main?dbc371eb2fab1daae2Both still show
MERGEDon GitHub, so the issues they closed looked handled while the code was gone. Confirmed against the source:sentryFilters.tshad only theEPIPE: broken pipespelling,stats.tshad theisReady()guard onrecord-shortcut-usageonly, andisExpectedGroomingFailuredid not exist. All three issues are live again on 0.17.3.Both PRs are restored here byte-for-byte (
git applyof their own diffs, plus cherry-pick of the6d3d22766regex follow-up) so the eventual rc -> main merge converges without conflict.Restored
EPIPE: broken pipe; Node emits a barewrite EPIPEfor stream/socket writes to a dead pipe, so the rule never fired.stats:record-session-created/-closedthrewDatabase not initializedwhen an agent was restored on boot ahead of the stats DB. Both callers are fire-and-forget and ignore the return value.sessions:setMany/setAllEACCES onmaestro-sessions.json.isExpectedGroomingFailure()for the group-chat groom path, including the follow-up that scopes the regex toAgent ... is not available.New this round
isExpectedGroomingFailure()to cover a groom process that would not launch (context-groomer.ts:393, reached fromgroupChat.ts) and revoked provider credentials. Both are user-environment conditions thatresetContextalready recovers from by starting a fresh session. Only the group-chat call site suppresses - the user-initiated groom paths incontext.tsanddirector-notes.tsstill surface, as in fix(sentry): don't report expected group-chat grooming failures (MAESTRO-KA) #1246.http 503. The sampler runs on a timer, so a single ChatGPT outage reports once per tick per install. Now skips 408/429/5xx as well; a 4xx implying we sent a malformed request still reports, because that would be our bug.isGitHubConnectivityErrormatched socket-level failures but not GitHub-side 5xx, soHTTP 504: 504 Gateway Timeout (https://api.github.com/graphql)paged Sentry on every Cue poll tick for the duration of a GitHub outage. The poller already retries on its own schedule.getGlobalStatsincremental loops report EACCES on transcripts under~/.claude/projectsthat belong to the agent CLI and that we merely discovered on disk. Extends the existingRangeErrorcarve-out to expected fs error codes, mirroringEXPECTED_FS_ERROR_CODESinshared-history-manager.ts.Deliberately not touched
GLIBC_2.38for better-sqlite3. Build/CI glibc target, not app code. rc PR ci(release): build Linux x64 on ubuntu-22.04 and guard the glibc floor #1290 (build Linux x64 on ubuntu-22.04 and guard the glibc floor) is the real fix.Preload script erroris a corrupt install (dist\main\preload.js not found in ...app.asar);window.maestrois then undefined and every unguarded call site throws. Optional chaining would mask a broken install across ~40 call sites.channel:stablebut their release is the Electron version (41.6.0) and the paths areAppData\Roaming\Electron, i.e. unpackaged dev runs, not the stable release line. TX and W1 are already fixed on rc in fix(sentry): stop paging on expected sidecar, bridge, and signal conditions #1247.spawn codex ENOENTwith no app frames in the stack. Genuine spawn failures are real signal.maestro-pexit semantics.Validation
git stash pushof the source files only, re-run, 27 red, restore, 206 green).npm run lint: 112 errors, identical to cleanmain(missing@codemirror/lang-*optional deps); zero in any changed file.main- no new failures.Summary by CodeRabbit