fix(sentry): repair rc IPC handler registration, bridge socket path, and routing noise - #1341
fix(sentry): repair rc IPC handler registration, bridge socket path, and routing noise#1341pedramamini wants to merge 1 commit into
Conversation
…and routing noise Four field issues from the rc channel (Sentry, release 0.18.4/0.18.5-RC). MAESTRO-YA - "Attempted to register a second handler for 'director-notes:getRichOverviewStats'". The 2026-07-31 back-merge of main into rc kept BOTH branches' copies of the rich-stats handler: rc's (which tallies AGENT-typed history entries) at line 651 and main's (which hardcodes that count to 0) at line 778. ipcMain.handle throws on the second registration, and the throw escapes registerDirectorNotesHandlers into setupIpcHandlers - so director-notes:generateSynopsis and every one of the ~40 register*Handlers calls after it (cue, agents, process, persistence, stats, ssh-remote, filesystem, ...) never registered. Removed the main-side duplicate; the rc version is the correct one for this branch. MAESTRO-WH - "listen EINVAL" starting the coworking bridge. The POSIX socket lived unconditionally at <userData>/coworking.sock, but sockaddr_un.sun_path is a fixed 104 bytes on macOS (108 on Linux). A deep userData directory produced a 123-byte path, bind() rejected it, and coworking silently stayed unavailable for that install. Long paths now fall back to a short temp-dir socket keyed by the same userData hash the Windows named pipe already uses. Both the bridge and the agent-facing env var resolve through the one function, so they stay in sync. MAESTRO-YB / MAESTRO-YC - "sshRemotes.find is not a function" surfacing as failed git:status and git:numstat invokes. electron-store only substitutes the [] default when the key is absent, so a hand-edited or sync-mangled settings.json hands getSshRemoteById a non-array and takes down callers that use no SSH remote at all. Treat an unusable value as "no remotes configured" and warn once so the bad file stays diagnosable. MAESTRO-M4 - "Group chat not found: <id>" reported from the exit listener. Participants keep running after the user deletes their group chat, so the later exit routes into a chat that no longer loads. The listener already recovers, and it retries once through the fallback path, so one deleted chat produced two Sentry events per participant. Skip reporting for exactly that message; every other routing failure still reports. Regression tests for all four, each confirmed failing against the unfixed source (14 red before, 54 green after).
📝 WalkthroughWalkthroughThe PR adds platform-aware coworking socket paths, filters expected deleted-group-chat errors from Sentry, hardens SSH remote lookup, and removes duplicate Director’s Notes IPC registration. ChangesSocket path resolution
Deleted group-chat error handling
SSH remote settings validation
Director’s Notes IPC registration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 SummaryThis PR repairs four rc-channel failures involving IPC registration, long coworking socket paths, malformed SSH settings, and expected group-chat exit noise.
Confidence Score: 4/5The malformed SSH settings recovery should be completed before merging because an array containing null or primitive entries still crashes canonical lookup callers. The primary fixes are well targeted and tested, but the new Files Needing Attention: src/main/stores/getters.ts and src/tests/main/stores/getters.test.ts Important Files Changed
Reviews (1): Last reviewed commit: "fix(sentry): repair rc IPC handler regis..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| return undefined; | ||
| } | ||
| return sshRemotes.find((r) => r.id === sshRemoteId); |
There was a problem hiding this comment.
If a hand-edited or sync-corrupted setting contains an array with null or primitive entries, Array.isArray passes but the callback dereferences r.id, causing git polling and other canonical getter callers to fail instead of treating the setting as unusable.
| return sshRemotes.find((r) => r.id === sshRemoteId); | |
| return sshRemotes.find( | |
| (r) => r !== null && typeof r === 'object' && r.id === sshRemoteId | |
| ); |
Context Used: CLAUDE.md (source)
Knowledge Base Used: Persistence: Settings, Sessions, and History
![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 (2)
src/__tests__/main/process-listeners/exit-listener-group-chat-noise.test.ts (1)
18-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnsure both Sentry filtering branches have integration coverage.
This file tests only
isDeletedGroupChatFailure. It does not execute the participant-exit catch atsrc/main/process-listeners/exit-listener.tsLine [478] or the fallback catch at Line [502]. Add assertions that deleted-group-chat errors are not sent to Sentry and unrelated routing errors are still reported.🤖 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/process-listeners/exit-listener-group-chat-noise.test.ts` around lines 18 - 52, Extend the tests beyond isDeletedGroupChatFailure to exercise both Sentry filtering branches in the participant-exit and fallback catch paths of exit-listener.ts. Assert that deleted-group-chat errors are excluded from Sentry while unrelated routing failures are reported, covering each branch through its integration-facing listener behavior.src/__tests__/main/stores/getters.test.ts (1)
220-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the single-warning contract.
The production code promises one
logger.warncall per process. This test checks only the return value. Add a focused case that callsgetSshRemoteByIdtwice with the same malformed value and asserts one warning. Isolate the test from the module-level warning state.🤖 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/stores/getters.test.ts` around lines 220 - 235, Extend the malformed sshRemotes tests around getSshRemoteById with a focused case that invokes it twice using the same invalid stored value and verifies logger.warn is called exactly once. Reset or otherwise isolate the module-level warning state before the case so the assertion is independent of other tests.
🤖 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/main/coworking/coworking-socket-path.ts`:
- Around line 45-51: Update the fallback path logic in the socket-path resolver
to validate the complete path against the POSIX socket path limit, including
os.tmpdir(). When it exceeds the limit, fall back to a known short POSIX
directory and shorter filename while preserving uniqueness; add a regression
test that mocks a long temporary directory and verifies the bounded result.
In `@src/main/stores/getters.ts`:
- Around line 135-144: Update the sshRemotes lookup in the getter containing the
malformed-array warning so its find callback validates each entry before reading
r.id, safely ignoring null or non-object entries while preserving valid remote
matching. Add a regression test covering an array containing a malformed entry
such as null and verify it does not throw.
---
Nitpick comments:
In `@src/__tests__/main/process-listeners/exit-listener-group-chat-noise.test.ts`:
- Around line 18-52: Extend the tests beyond isDeletedGroupChatFailure to
exercise both Sentry filtering branches in the participant-exit and fallback
catch paths of exit-listener.ts. Assert that deleted-group-chat errors are
excluded from Sentry while unrelated routing failures are reported, covering
each branch through its integration-facing listener behavior.
In `@src/__tests__/main/stores/getters.test.ts`:
- Around line 220-235: Extend the malformed sshRemotes tests around
getSshRemoteById with a focused case that invokes it twice using the same
invalid stored value and verifies logger.warn is called exactly once. Reset or
otherwise isolate the module-level warning state before the case so the
assertion is independent of other tests.
🪄 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: c63ffe35-47ab-49d7-a052-856052726ab8
📒 Files selected for processing (8)
src/__tests__/main/coworking/coworking-socket-path.test.tssrc/__tests__/main/ipc/handlers/directorNotes-richStats.test.tssrc/__tests__/main/process-listeners/exit-listener-group-chat-noise.test.tssrc/__tests__/main/stores/getters.test.tssrc/main/coworking/coworking-socket-path.tssrc/main/ipc/handlers/director-notes.tssrc/main/process-listeners/exit-listener.tssrc/main/stores/getters.ts
💤 Files with no reviewable changes (1)
- src/main/ipc/handlers/director-notes.ts
| // The userData path is too deep to hold a bindable socket (long home | ||
| // directory, nested portable/test data dir, ...). Fall back to a short path | ||
| // under the temp dir, keyed by the same userData hash the Windows pipe uses | ||
| // so the socket stays unique per data directory. Callers all resolve the | ||
| // path through this function, so the bridge and the env var agents read | ||
| // (COWORKING_SOCKET_ENV_VAR) stay in sync automatically. | ||
| return path.join(os.tmpdir(), `maestro-coworking-${userDataSlug(userData)}.sock`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the fallback socket path before returning it.
Line 51 assumes that os.tmpdir() is short. os.tmpdir() can use a long environment-configured path. In that case, the fallback still exceeds SUN_PATH_MAX and the bridge cannot bind its socket.
Check the complete fallback path. If it is too long, use a known short POSIX directory and a shorter filename. Add a regression test with a mocked long temporary directory.
Proposed fix
- return path.join(os.tmpdir(), `maestro-coworking-${userDataSlug(userData)}.sock`);
+ const slug = userDataSlug(userData);
+ const fallback = path.join(os.tmpdir(), `maestro-coworking-${slug}.sock`);
+ if (Buffer.byteLength(fallback) + 1 <= SUN_PATH_MAX) return fallback;
+
+ return path.join('/tmp', `m-${slug}.sock`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The userData path is too deep to hold a bindable socket (long home | |
| // directory, nested portable/test data dir, ...). Fall back to a short path | |
| // under the temp dir, keyed by the same userData hash the Windows pipe uses | |
| // so the socket stays unique per data directory. Callers all resolve the | |
| // path through this function, so the bridge and the env var agents read | |
| // (COWORKING_SOCKET_ENV_VAR) stay in sync automatically. | |
| return path.join(os.tmpdir(), `maestro-coworking-${userDataSlug(userData)}.sock`); | |
| // The userData path is too deep to hold a bindable socket (long home | |
| // directory, nested portable/test data dir, ...). Fall back to a short path | |
| // under the temp dir, keyed by the same userData hash the Windows pipe uses | |
| // so the socket stays unique per data directory. Callers all resolve the | |
| // path through this function, so the bridge and the env var agents read | |
| // (COWORKING_SOCKET_ENV_VAR) stay in sync automatically. | |
| const slug = userDataSlug(userData); | |
| const fallback = path.join(os.tmpdir(), `maestro-coworking-${slug}.sock`); | |
| if (Buffer.byteLength(fallback) + 1 <= SUN_PATH_MAX) return fallback; | |
| return path.join('/tmp', `m-${slug}.sock`); |
🤖 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/coworking/coworking-socket-path.ts` around lines 45 - 51, Update the
fallback path logic in the socket-path resolver to validate the complete path
against the POSIX socket path limit, including os.tmpdir(). When it exceeds the
limit, fall back to a known short POSIX directory and shorter filename while
preserving uniqueness; add a regression test that mocks a long temporary
directory and verifies the bounded result.
| if (!Array.isArray(sshRemotes)) { | ||
| if (!warnedAboutMalformedSshRemotes) { | ||
| warnedAboutMalformedSshRemotes = true; | ||
| logger.warn( | ||
| `Ignoring malformed 'sshRemotes' setting (expected an array, got ${typeof sshRemotes})`, | ||
| 'Settings' | ||
| ); | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard malformed entries inside the array.
Array.isArray(sshRemotes) validates only the container. A hand-edited settings.json can contain [null], and the subsequent .find() callback still dereferences r.id. This causes another TypeError and leaves the Sentry failure unresolved. Guard each entry before reading id, and add a regression case for a malformed array entry.
Proposed guard
- return sshRemotes.find((r) => r.id === sshRemoteId);
+ return sshRemotes.find((r) => r?.id === sshRemoteId);🤖 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/stores/getters.ts` around lines 135 - 144, Update the sshRemotes
lookup in the getter containing the malformed-array warning so its find callback
validates each entry before reading r.id, safely ignoring null or non-object
entries while preserving valid remote matching. Add a regression test covering
an array containing a malformed entry such as null and verify it does not throw.
1 participant
Sentry triage on the
rcchannel. Four issues, all confirmed against current rc code and releases 0.18.4-RC / 0.18.5-RC.MAESTRO-YA - duplicate IPC handler aborts startup registration (the important one)
Error: Attempted to register a second handler for 'director-notes:getRichOverviewStats'- 9 events, first seen ~19h after the back-merge landed, release 0.18.5-RC (= currentpackage.jsonat HEAD).070c2eee3 merge: back-merge origin/main into rc(2026-07-31) resolveddb3c65946by keeping both branches' copies of the rich-stats handler:AGENT-typed history entries (b6a052c39gave cross-agent consults their own entry type)agentEntryCount = 0with a stale// Generate AI synopsis via batch-mode agentcommentipcMain.handlethrows on the second registration of a channel, and that throw escapesregisterDirectorNotesHandlersintosetupIpcHandlers()(no try/catch). Everything after it is skipped:director-notes:generateSynopsis, plus the ~40register*Handlerscalls that follow atindex.ts:3338+- cue, agents, process, persistence, system, claude, group chat, plugins, coworking, windows, stats, ssh-remote, filesystem, notifications and the rest.Removed the main-side duplicate. The rc version is the correct one for this branch.
MAESTRO-WH - coworking bridge unusable when userData is deep
Error: listen EINVAL: invalid argument .../coworking.sock- 9 events,channel:rc.getBridgeSocketPath()returned<userData>/coworking.sockunconditionally on POSIX, butsockaddr_un.sun_pathis a fixed-size array: 104 bytes on macOS/BSD, 108 on Linux. The reported path is 123 bytes, sobind()rejected the address and coworking silently stayed unavailable for that install for good.Over the limit, the socket now falls back to a short temp-dir path keyed by the same userData SHA-1 the Windows named pipe already uses (Windows solved this exact problem by hashing). The bridge and the agent-facing
COWORKING_SOCKET_ENV_VARboth resolve through the one function, so they stay in sync automatically.MAESTRO-YB / MAESTRO-YC -
sshRemotes.find is not a functionSurfaced as failed
git:statusandgit:numstatinvokes,channel:rc, release 0.18.4-RC.settings.jsonis a plain file in userData that users hand-edit and sync tools rewrite, and electron-store only substitutes the[]default when the key is absent - not when it holds a non-array. A malformed value took down every caller of the canonicalgetSshRemoteById(), including the Right Bar's git polls on agents that use no SSH remote at all.Treat an unusable value as "no remotes configured" and
logger.warnonce so the bad file stays diagnosable rather than silently swallowed.Note: five other sites read
sshRemoteswith their own inline copy of this lookup (marketplace.ts,agents.ts,autorun.ts,web-server-factory.ts,ssh-remote-resolver.ts). None of them has reported a crash, so they are left alone here - flagging for a future dedup pass.MAESTRO-M4 -
Group chat not foundreported as a crashParticipants keep running after the user deletes their group chat, so the exit that fires later routes into a chat
loadGroupChatcan no longer find. The exit listener already recovers (participant marked done, buffer cleared) and retries once through the fallback path, so a single deleted chat produced two Sentry events per participant.Follows the existing
isExpectedGroomingFailureconvention: a narrow predicate, applied only at the two exit-listener catches. Deliberately does not matchParticipant '...' not found(the chat still exists) orGroup chat not found after participant update(that one means a broken write).Tests
19 new assertions across 4 files. Every one confirmed failing against the unfixed source per the usual vacuity check (
git stash push -- <source only>): 14 red before the fix, 54 green after.The MAESTRO-YA test needed two angles - the existing suite's
ipcMainmock is aMap, so a duplicate registration just overwrites the entry and goes unnoticed (which is why the bug shipped with tests passing). One test asserts on the raw call list, another re-runs registration against a mock that throws like the realipcMain.Validation
npx tsc --noEmit: 0 errorscoworking/,stores/, director-notes, git, groupChat, process-listeners, coworking-injection): 621 passed, 0 failedexamples/plugins/agent-flow/main.js, untouched by this branchInvestigated and deliberately skipped
MarketplaceFetchError: Network error fetching manifest: fetch failed) - already fixed, just unshipped. ThesentryFilters.tsrule matches, and the LinkedErrors haystack fix that made it live is6e5236a1c(2026-07-14).git merge-base --is-ancestor 6e5236a1c v0.18.4-RC= false, and all events are on 0.18.4-RC or stable 0.17.3. Re-fixing correct code was avoided.sessions:setManyEBADF) - 1 event.EBADFonopencan indicate a real fd leak, so adding it to the existing ENOSPC/EACCES/EPERM IPC-write rule could mask a genuine bug. Needs more signal first.gh pr listfailed) - per-issue breakdown ischannel:stable, release 0.17.3. Not an rc issue.maestro-p --status) - deferred a 6th time; still needs a human call on maestro-p exit semantics.Window unresponsive(62), renderer crashes (5A), minifiedvendor-reactissues with no app frames (Y2/Y3 - and Y2 is the[Splash]twin of Y3), spawn EPERM/EINVAL/EFTYPE (NM, R0, X4 - genuine spawn signal).Summary by CodeRabbit