Keep integration streams alive across token refresh#120
Conversation
|
CodeAnt AI is reviewing your PR. |
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. Warning Review limit reached
More reviews will be available in 30 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR establishes Slack-specific writeback command root path detection and validation, enriches integration and cloud agent mounts with explicit configuration for local layout and sync modes, introduces a token provider adapter for RelayFileSync initialization, and implements a launcher wrapper that injects mount configuration as environment variables during startup. ChangesIntegration mount configuration and launch flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
codeant-ai
Bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new module slack-writeback-command-roots.ts to canonicalize and validate Slack writeback command paths, replacing the legacy .outbox naming convention with specific subpaths like /messages and /replies. It also updates the mounting logic in integration-mounts.ts and cloud-agent.ts to support configurable localLayout and syncMode options (such as write-only for Slack writebacks). Feedback on these changes includes resolving a potential TypeScript strict mode compilation error in toErrorMessage by casting the record, replacing Object.assign with the object spread operator for better readability, and adding defensive runtime type checks for string parameters in the new utility functions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (isRecord(error)) { | ||
| const message = error.message | ||
| if (typeof message === 'string' && message.trim()) return message.trim() | ||
| const reason = error.reason | ||
| if (typeof reason === 'string' && reason.trim()) return reason.trim() | ||
| const status = error.status ?? error.statusCode ?? error.httpStatus | ||
| const code = error.code | ||
| const type = error.type | ||
| const parts = [ | ||
| typeof type === 'string' && type.trim() ? type.trim() : null, | ||
| typeof code === 'string' && code.trim() ? `code=${code.trim()}` : typeof code === 'number' ? `code=${code}` : null, | ||
| typeof status === 'string' && status.trim() ? `status=${status.trim()}` : typeof status === 'number' ? `status=${status}` : null | ||
| ].filter((entry): entry is string => entry !== null) | ||
| if (parts.length > 0) return parts.join(' ') | ||
| const constructorName = (error as { constructor?: { name?: string } }).constructor?.name | ||
| if (constructorName && constructorName !== 'Object') return constructorName | ||
| } |
There was a problem hiding this comment.
Accessing properties like message, reason, status, etc. directly on error after isRecord(error) check will cause TypeScript compilation errors under strict mode, because isRecord returns Record<string, unknown>. Casting error to Record<string, any> resolves this issue cleanly.
| if (isRecord(error)) { | |
| const message = error.message | |
| if (typeof message === 'string' && message.trim()) return message.trim() | |
| const reason = error.reason | |
| if (typeof reason === 'string' && reason.trim()) return reason.trim() | |
| const status = error.status ?? error.statusCode ?? error.httpStatus | |
| const code = error.code | |
| const type = error.type | |
| const parts = [ | |
| typeof type === 'string' && type.trim() ? type.trim() : null, | |
| typeof code === 'string' && code.trim() ? `code=${code.trim()}` : typeof code === 'number' ? `code=${code}` : null, | |
| typeof status === 'string' && status.trim() ? `status=${status.trim()}` : typeof status === 'number' ? `status=${status}` : null | |
| ].filter((entry): entry is string => entry !== null) | |
| if (parts.length > 0) return parts.join(' ') | |
| const constructorName = (error as { constructor?: { name?: string } }).constructor?.name | |
| if (constructorName && constructorName !== 'Object') return constructorName | |
| } | |
| if (isRecord(error)) { | |
| const record = error as Record<string, any> | |
| const message = record.message | |
| if (typeof message === 'string' && message.trim()) return message.trim() | |
| const reason = record.reason | |
| if (typeof reason === 'string' && reason.trim()) return reason.trim() | |
| const status = record.status ?? record.statusCode ?? record.httpStatus | |
| const code = record.code | |
| const type = record.type | |
| const parts = [ | |
| typeof type === 'string' && type.trim() ? type.trim() : null, | |
| typeof code === 'string' && code.trim() ? `code=${code.trim()}` : typeof code === 'number' ? `code=${code}` : null, | |
| typeof status === 'string' && status.trim() ? `status=${status.trim()}` : typeof status === 'number' ? `status=${status}` : null | |
| ].filter((entry): entry is string => entry !== null) | |
| if (parts.length > 0) return parts.join(' ') | |
| const constructorName = record.constructor?.name | |
| if (constructorName && constructorName !== 'Object') return constructorName | |
| } |
| private createContractLauncher(spec: IntegrationMountSpec, launcher: MountLauncher): MountLauncher { | ||
| return Object.assign({ | ||
| start: (input: MountLauncherStart) => launcher.start({ | ||
| ...input, | ||
| env: { | ||
| ...input.env, | ||
| RELAYFILE_MOUNT_LOCAL_LAYOUT: spec.localLayout, | ||
| RELAYFILE_MOUNT_SYNC_MODE: spec.syncMode | ||
| } | ||
| }) | ||
| }, { | ||
| __options: (launcher as { __options?: unknown }).__options | ||
| }) as MountLauncher | ||
| } |
There was a problem hiding this comment.
Using the object spread operator is much cleaner, more readable, and less error-prone than Object.assign with multiple object literals. It automatically preserves __options and any other properties on launcher without needing explicit casting.
private createContractLauncher(spec: IntegrationMountSpec, launcher: MountLauncher): MountLauncher {
return {
...launcher,
start: (input: MountLauncherStart) => launcher.start({
...input,
env: {
...input.env,
RELAYFILE_MOUNT_LOCAL_LAYOUT: spec.localLayout,
RELAYFILE_MOUNT_SYNC_MODE: spec.syncMode
}
})
}
}| function normalizeRemotePath(path: string): string | null { | ||
| const segments = path | ||
| .trim() | ||
| .split(/[\\/]+/) | ||
| .filter((segment) => segment.length > 0) | ||
| if (segments.some((segment) => segment === '.' || segment === '..')) return null | ||
| return segments.length > 0 ? `/${segments.join('/')}` : '/' | ||
| } |
There was a problem hiding this comment.
Add a defensive type check to ensure path is a string before calling .trim(). This prevents potential runtime crashes if path is unexpectedly null, undefined, or of another type.
| function normalizeRemotePath(path: string): string | null { | |
| const segments = path | |
| .trim() | |
| .split(/[\\/]+/) | |
| .filter((segment) => segment.length > 0) | |
| if (segments.some((segment) => segment === '.' || segment === '..')) return null | |
| return segments.length > 0 ? `/${segments.join('/')}` : '/' | |
| } | |
| function normalizeRemotePath(path: string): string | null { | |
| if (typeof path !== 'string') return null | |
| const segments = path | |
| .trim() | |
| .split(/[\\/]+/) | |
| .filter((segment) => segment.length > 0) | |
| if (segments.some((segment) => segment === '.' || segment === '..')) return null | |
| return segments.length > 0 ? `/${segments.join('/')}` : '/' | |
| } |
| function isSlackProvider(provider: string): boolean { | ||
| const normalized = provider.trim().toLowerCase() | ||
| return normalized === 'slack' || normalized.startsWith('slack-') | ||
| } |
There was a problem hiding this comment.
Add a defensive type check to ensure provider is a string before calling .trim(). This prevents potential runtime crashes if provider is unexpectedly null, undefined, or of another type.
| function isSlackProvider(provider: string): boolean { | |
| const normalized = provider.trim().toLowerCase() | |
| return normalized === 'slack' || normalized.startsWith('slack-') | |
| } | |
| function isSlackProvider(provider: string): boolean { | |
| if (typeof provider !== 'string') return false | |
| const normalized = provider.trim().toLowerCase() | |
| return normalized === 'slack' || normalized.startsWith('slack-') | |
| } |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const normalized = normalizeRemotePath(mountPath) | ||
| if (!normalized) return null | ||
| const segments = normalized.split('/').filter(Boolean) | ||
| if (segments[0] !== 'slack') return null |
There was a problem hiding this comment.
Suggestion: The root-segment check is hardcoded to slack, so providers that are intentionally treated as Slack-compatible (for example IDs starting with slack-) will always return null when their mount paths are rooted at /<provider>/.... This breaks command-root derivation and can silently disable expected local writeback/watch behavior for those integrations. Use the normalized provider root (or explicitly support all Slack-compatible roots) instead of a fixed 'slack' segment check. [logic error]
Severity Level: Major ⚠️
- ❌ Slack-* providers never get write-only Slack command mounts.
- ⚠️ Local watch roots skipped for Slack-compatible integrations.
- ⚠️ Event bridge cannot bound Slack-like command roots.Steps of Reproduction ✅
1. A connected integration is hydrated from cloud with `provider = 'slack-enterprise'` and
a mount path like `/integrations/slack-enterprise/channels/C123` (provider naming scheme
is explicitly supported by `isSlackProvider`, see
`/workspace/pear/src/main/slack-writeback-command-roots.ts:12-15` and
`/workspace/pear/src/main/integration-event-bridge.ts:13-16` which treat any `slack-*`
provider as Slack-compatible).
2. `IntegrationsManager.syncLocalMounts()` in
`/workspace/pear/src/main/integrations.ts:1912-1932` builds local entries and, for each
visible integration, calls `integrationMountManager.ensureMounted()` with `{ provider:
integration.provider, mountPaths: this.mountPathsForLocalSync(integration) }`, so the
Slack-compatible integration is passed through with `provider = 'slack-enterprise'`.
3. `mountPathsForLocalSync()` (integrations.ts:1748-1753) delegates to
`localSyncMountPathsForIntegration()`, which uses
`canonicalMountPathsForConnectedIntegration()` plus `rewriteIntegrationMountPath()`
(integrations.ts:38-47, 61-66, 69-71) to normalize catalog paths; for a Slack-compatible
provider, a path like `/integrations/slack-enterprise/channels/C123` becomes a root-level
path `'/slack-enterprise/channels/C123'`, which is then included in `mountPaths` passed to
`IntegrationMountManager`.
4. Inside `IntegrationMountManager.mount()` in
`/workspace/pear/src/main/integration-mounts.ts:234-260`, `mountSpecsFor()` is invoked
(lines 84-92 in the same file) to build Relayfile mount specs; for each `mountPath` (e.g.
`'/slack-enterprise/channels/C123/messages'`), it computes `syncMode:
isSlackWritebackCommandRoot(mountPath) ? 'write-only' : 'mirror'`.
5. `isSlackWritebackCommandRoot()` in
`/workspace/pear/src/main/slack-writeback-command-roots.ts:42-44` normalizes the path and
calls `slackWritebackCommandMountPathFor('slack', remotePath)`, hardcoding the provider
argument to `'slack'` while the normalized path segments start with `'slack-enterprise'`.
6. `slackWritebackCommandMountPathFor()` (slack-writeback-command-roots.ts:17-39) first
confirms the provider is Slack-compatible (`isSlackProvider('slack')` is true) and then
normalizes `mountPath` into segments like
`['slack-enterprise','channels','C123','messages']`, but the hardcoded check `if
(segments[0] !== 'slack') return null` at line 22 causes the function to return `null` for
any Slack-compatible provider whose root segment is not literally `'slack'`.
7. Because `slackWritebackCommandMountPathFor('slack', remotePath)` returns `null`,
`isSlackWritebackCommandRoot(remotePath)` returns `false` even for valid Slack-compatible
channel message roots, so `syncMode` in `mountSpecsFor()` falls back to `'mirror'` instead
of `'write-only'` and the path is not treated as a bounded command root.
8. The same hardcoded `segments[0] !== 'slack'` check also affects the event bridge:
`allowsLocalMountWatching()` and `localWatchRootsFor()` in
`/workspace/pear/src/main/integration-event-bridge.ts:77-85, 820-835` both rely on
`isBoundedLocalCommandRoot()` → `isSlackWritebackCommandRoot()` to identify Slack command
roots; for Slack-compatible providers whose root segment is `/slack-*/...`, no paths ever
satisfy `isSlackWritebackCommandRoot`, so local watch roots and bounded command-root
behavior are silently disabled for those integrations while they still work for the plain
`'slack'` provider.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/slack-writeback-command-roots.ts
**Line:** 22:22
**Comment:**
*Logic Error: The root-segment check is hardcoded to `slack`, so providers that are intentionally treated as Slack-compatible (for example IDs starting with `slack-`) will always return `null` when their mount paths are rooted at `/<provider>/...`. This breaks command-root derivation and can silently disable expected local writeback/watch behavior for those integrations. Use the normalized provider root (or explicitly support all Slack-compatible roots) instead of a fixed `'slack'` segment check.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
|
Implemented and verified scoped fixes for PR #120. Changed:
Verification run:
I did not end with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/slack-writeback-command-roots.ts (1)
1-46: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd regression tests for Slack writeback command root logic.
This new module implements non-trivial path normalization and mapping logic that affects integration mount configuration and event streaming. The coding guideline requires regression tests when touching integration notifications or event streaming. Tests should cover:
- Provider detection edge cases (case sensitivity, prefix matching)
- Path normalization edge cases (empty paths, trailing slashes, mixed separators, path traversal attempts)
- Mount path mapping for all supported patterns (channels, dms, users; messages, threads/replies)
- Canonical path detection (distinguishing complete vs. incomplete paths)
- Invalid path rejection (unsupported collections, malformed structures)
As per coding guidelines: "Add regression tests when touching broker start, event streaming, PTY buffering, spawned personas, or integration notifications. Include duplicate/replay cases, not just the happy path"
🤖 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/slack-writeback-command-roots.ts` around lines 1 - 46, The new module lacks regression tests for the non-trivial path normalization and mapping logic; add unit tests that exercise isSlackProvider, normalizeRemotePath, slackWritebackCommandMountPathFor, and isSlackWritebackCommandRoot covering provider detection edge cases (case variants and "slack-" prefix), path normalization edge cases (empty input, trailing slashes, backslashes vs forward slashes, segments with "." or ".."), all valid mount patterns for collections ('channels','dms','users') mapping to messages/threads/replies (3/4/5/6-segment cases), canonical-vs-incomplete detection (where slackWritebackCommandMountPathFor returns normalized vs null), and invalid rejections (unsupported collections, malformed structures); create tests that assert exact returned normalized paths or null and include duplicate/replay cases per the guideline.Source: Coding guidelines
🧹 Nitpick comments (2)
src/main/slack-writeback-command-roots.ts (1)
21-21: 💤 Low valueOptional: Remove redundant
.filter(Boolean)call.The
normalizedpath returned fromnormalizeRemotePath()already filters empty segments (line 7), so the.filter(Boolean)here is redundant. While harmless, removing it would improve clarity.♻️ Proposed simplification
- const segments = normalized.split('/').filter(Boolean) + const segments = normalized.split('/').slice(1)Note: Using
.slice(1)removes the leading empty string from splitting a path starting with/, which is equivalent to.filter(Boolean)in this context.🤖 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/slack-writeback-command-roots.ts` at line 21, The segments variable currently applies .filter(Boolean) after splitting normalized, but normalizeRemotePath() already strips empty segments; remove the redundant filter by changing the assignment for segments (the const named segments that uses normalized.split('/').filter(Boolean)) to just normalized.split('/') or, if you want to explicitly drop a leading empty entry, use normalized.split('/').slice(1); keep references to normalized and normalizeRemotePath() intact.src/main/__tests__/integration-event-bridge.test.ts (1)
247-259: ⚡ Quick winExercise token rotation on reconnect, not just adapter plumbing.
This only verifies that the function reference is forwarded. The regression here is stale tokens after reconnect, so the valuable test is one where the provider returns a different token on a later start/reconnect and the stream uses the new value. As per coding guidelines, event-streaming changes should carry regression coverage beyond the happy path, and this PR’s stated bug is reconnect behavior after token refresh.
🤖 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/__tests__/integration-event-bridge.test.ts` around lines 247 - 259, The test currently only checks that integrationRelayFileSyncOptions forwards the tokenProvider reference; instead change it to exercise token rotation by using a tokenProvider that returns different tokens over time (e.g., a closure or mock that yields 'token-v1' then 'token-v2'), instantiate the stream/options via integrationRelayFileSyncOptions, start or simulate a reconnect/restart of the relay stream (the code path that re-reads options.token on reconnect), and assert that the stream uses the new token after reconnect; specifically ensure you call the tokenProvider through the same mechanism used by the relay (not just inspect options.token) so the test covers reconnect behavior and stale-token regression.Sources: Coding guidelines, Learnings
🤖 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/cloud-agent.test.ts`:
- Around line 397-416: The test currently asserts mount input paths but omits
verifying that the launcher environment variables are propagated
(RELAYFILE_MOUNT_LOCAL_LAYOUT / RELAYFILE_MOUNT_SYNC_MODE). Update the test for
CloudAgentManager.attach to also assert that the launcher/env payload produced
by the mock (e.g., mock.mountInputs[0].env or mock.launcherEnv depending on how
the mock surfaces env) contains keys RELAYFILE_MOUNT_LOCAL_LAYOUT and
RELAYFILE_MOUNT_SYNC_MODE with values 'exact' and 'mirror' respectively so
regression of env injection is caught; locate this check near the existing
assertions that examine mock.mountInputs[0] in the test.
In `@src/main/integration-event-bridge.ts`:
- Around line 278-299: The toErrorMessage function currently returns immediately
for Error instances which yields an empty string for errors with blank message;
change this so Error instances still participate in the structured fallback: in
toErrorMessage, replace the early return on "if (error instanceof Error) return
error.message" with logic that reads and trims error.message and only returns it
if non-empty, otherwise continue to the existing isRecord/structured extraction
(status/code/type/constructorName) so custom Error subclasses or wrappers
surface those fields; keep the same downstream behavior for non-Error records
and the final stringification fallback.
In `@src/main/integrations.test.ts`:
- Around line 321-337: Add a regression test to integrations.test.ts that calls
localSyncMountPathsForIntegration with a mountPaths array containing both legacy
and canonical duplicates (e.g., '/slack/channels', '/slack/channels/C123', and
'/slack/channels/C123/messages' plus similar legacy '/slack/dms/D123' vs
'/slack/dms/D123/messages') and assert the result deduplicates/replays to only
the canonical writeback roots; keep the same other args used in the existing
case (provider: 'slack', integrationId, connectedAt, notifyAgent,
subscribeAgent, downloadHistoricalData: false) so you lock down dedupe behavior
for localSyncMountPathsForIntegration when mountPaths contains mixed
legacy+canonical entries.
In `@src/main/slack-writeback-command-roots.ts`:
- Around line 12-15: The helper function isSlackProvider should be made
available to other modules so they can reuse the same provider-detection logic;
update the declaration of isSlackProvider to be a named export (export function
isSlackProvider(...)) and ensure any module that currently duplicates this logic
(e.g., integration-event-bridge) imports this named export instead of
reimplementing it; keep the function signature and behavior unchanged to
maintain compatibility.
---
Outside diff comments:
In `@src/main/slack-writeback-command-roots.ts`:
- Around line 1-46: The new module lacks regression tests for the non-trivial
path normalization and mapping logic; add unit tests that exercise
isSlackProvider, normalizeRemotePath, slackWritebackCommandMountPathFor, and
isSlackWritebackCommandRoot covering provider detection edge cases (case
variants and "slack-" prefix), path normalization edge cases (empty input,
trailing slashes, backslashes vs forward slashes, segments with "." or ".."),
all valid mount patterns for collections ('channels','dms','users') mapping to
messages/threads/replies (3/4/5/6-segment cases), canonical-vs-incomplete
detection (where slackWritebackCommandMountPathFor returns normalized vs null),
and invalid rejections (unsupported collections, malformed structures); create
tests that assert exact returned normalized paths or null and include
duplicate/replay cases per the guideline.
---
Nitpick comments:
In `@src/main/__tests__/integration-event-bridge.test.ts`:
- Around line 247-259: The test currently only checks that
integrationRelayFileSyncOptions forwards the tokenProvider reference; instead
change it to exercise token rotation by using a tokenProvider that returns
different tokens over time (e.g., a closure or mock that yields 'token-v1' then
'token-v2'), instantiate the stream/options via integrationRelayFileSyncOptions,
start or simulate a reconnect/restart of the relay stream (the code path that
re-reads options.token on reconnect), and assert that the stream uses the new
token after reconnect; specifically ensure you call the tokenProvider through
the same mechanism used by the relay (not just inspect options.token) so the
test covers reconnect behavior and stale-token regression.
In `@src/main/slack-writeback-command-roots.ts`:
- Line 21: The segments variable currently applies .filter(Boolean) after
splitting normalized, but normalizeRemotePath() already strips empty segments;
remove the redundant filter by changing the assignment for segments (the const
named segments that uses normalized.split('/').filter(Boolean)) to just
normalized.split('/') or, if you want to explicitly drop a leading empty entry,
use normalized.split('/').slice(1); keep references to normalized and
normalizeRemotePath() intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef8844db-cdbe-4914-8935-0058625a3ee1
📒 Files selected for processing (9)
src/main/__tests__/integration-event-bridge.test.tssrc/main/cloud-agent.test.tssrc/main/cloud-agent.tssrc/main/integration-event-bridge.tssrc/main/integration-mounts.test.tssrc/main/integration-mounts.tssrc/main/integrations.test.tssrc/main/integrations.tssrc/main/slack-writeback-command-roots.ts
| it('mounts non-root sandbox relayfile paths at the project root without local double-pathing', async () => { | ||
| mock.boxResponses.push({ | ||
| sandboxId: 'sandbox-1', | ||
| execUrl: 'https://sandbox.example', | ||
| relayfileToken: 'relayfile-token', | ||
| relayfileMountPath: '/workspace/project-1', | ||
| status: 'ready' | ||
| }) | ||
| const manager = new CloudAgentManager() | ||
|
|
||
| await manager.attach('project-1', 'cloud-agent-1') | ||
|
|
||
| expect(mock.mountInputs[0]).toMatchObject({ | ||
| localDir: '/tmp/project-1', | ||
| remotePath: '/workspace/project-1', | ||
| localLayout: 'exact', | ||
| syncMode: 'mirror' | ||
| }) | ||
| expect(mock.mountInputs[0]?.localDir).not.toContain('/workspace/project-1') | ||
| }) |
There was a problem hiding this comment.
Add a regression assertion for launcher env propagation.
This new test validates mount input, but not the launcher wrapper contract (RELAYFILE_MOUNT_LOCAL_LAYOUT / RELAYFILE_MOUNT_SYNC_MODE). A break in env injection would currently go undetected.
Proposed test hardening
type MountInput = {
workspaceId: string
localDir: string
remotePath: string
localLayout?: string
syncMode?: string
scopes?: string[]
+ launcher?: {
+ start: (input: { env?: Record<string, string>; onEvent?: (event: { type: string }) => void }) => unknown
+ }
}
+const pearLauncherStart = vi.fn(async (_input: unknown) => undefined)
class RelayfileSetup {
mountWorkspace(input: MountInput) {
mountInputs.push(input)
+ void input.launcher?.start({
+ env: { EXISTING_ENV: '1' },
+ onEvent: () => undefined
+ })
return Promise.resolve({
stop: vi.fn(async () => undefined),
status: vi.fn(async () => ({ state: 'running' }))
})
}
}
// ...
vi.mock('./relayfile-mount-launcher', () => ({
- createPearMountLauncher: vi.fn(() => ({ start: vi.fn() }))
+ createPearMountLauncher: vi.fn(() => ({ start: pearLauncherStart }))
}))
// in the new test
+expect(pearLauncherStart).toHaveBeenCalledWith(expect.objectContaining({
+ env: expect.objectContaining({
+ RELAYFILE_MOUNT_LOCAL_LAYOUT: 'exact',
+ RELAYFILE_MOUNT_SYNC_MODE: 'mirror'
+ })
+}))As per coding guidelines, **/*.test.{js,ts,tsx} changes touching startup/streaming-adjacent behavior should include regression coverage beyond the happy path.
🤖 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/cloud-agent.test.ts` around lines 397 - 416, The test currently
asserts mount input paths but omits verifying that the launcher environment
variables are propagated (RELAYFILE_MOUNT_LOCAL_LAYOUT /
RELAYFILE_MOUNT_SYNC_MODE). Update the test for CloudAgentManager.attach to also
assert that the launcher/env payload produced by the mock (e.g.,
mock.mountInputs[0].env or mock.launcherEnv depending on how the mock surfaces
env) contains keys RELAYFILE_MOUNT_LOCAL_LAYOUT and RELAYFILE_MOUNT_SYNC_MODE
with values 'exact' and 'mirror' respectively so regression of env injection is
caught; locate this check near the existing assertions that examine
mock.mountInputs[0] in the test.
Sources: Coding guidelines, Learnings
| it('keeps local sync to discovery and narrow canonical writeback command roots while historical download is off', () => { | ||
| expect(localSyncMountPathsForIntegration({ | ||
| provider: 'slack', | ||
| integrationId: 'slack-integration-1', | ||
| scope: {}, | ||
| mountPaths: ['/discovery/slack', '/slack/channels', '/slack/channels/C123', '/slack/dms/D123'], | ||
| mountPaths: ['/discovery/slack', '/slack/channels', '/slack/channels/C123', '/slack/dms/D123', '/slack/users/U123/messages'], | ||
| connectedAt: '2026-06-05T00:00:00.000Z', | ||
| notifyAgent: true, | ||
| subscribeAgent: true, | ||
| downloadHistoricalData: false | ||
| })).toEqual([ | ||
| '/discovery/slack', | ||
| '/slack/channels/C123/.outbox', | ||
| '/slack/dms/D123/.outbox' | ||
| '/slack/channels/C123/messages', | ||
| '/slack/dms/D123/messages', | ||
| '/slack/users/U123/messages' | ||
| ]) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a duplicate/replay regression for canonicalized Slack writeback roots.
Line 326 and Line 331 currently assert only happy-path canonicalization. Please add a case where mountPaths includes both legacy + canonical forms and duplicates, so replay/dedupe behavior is locked down.
Suggested test addition
+ it('dedupes replayed legacy/canonical Slack writeback roots when historical download is off', () => {
+ expect(localSyncMountPathsForIntegration({
+ provider: 'slack',
+ integrationId: 'slack-integration-1',
+ scope: {},
+ mountPaths: [
+ '/slack/channels/C123',
+ '/slack/channels/C123/messages',
+ '/slack/channels/C123'
+ ],
+ connectedAt: '2026-06-05T00:00:00.000Z',
+ notifyAgent: true,
+ subscribeAgent: true,
+ downloadHistoricalData: false
+ })).toEqual(['/slack/channels/C123/messages'])
+ })As per coding guidelines, “Add regression tests when touching broker start, event streaming, PTY buffering, spawned personas, or integration notifications. Include duplicate/replay cases, not just the happy path.”
🤖 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/integrations.test.ts` around lines 321 - 337, Add a regression test
to integrations.test.ts that calls localSyncMountPathsForIntegration with a
mountPaths array containing both legacy and canonical duplicates (e.g.,
'/slack/channels', '/slack/channels/C123', and '/slack/channels/C123/messages'
plus similar legacy '/slack/dms/D123' vs '/slack/dms/D123/messages') and assert
the result deduplicates/replays to only the canonical writeback roots; keep the
same other args used in the existing case (provider: 'slack', integrationId,
connectedAt, notifyAgent, subscribeAgent, downloadHistoricalData: false) so you
lock down dedupe behavior for localSyncMountPathsForIntegration when mountPaths
contains mixed legacy+canonical entries.
Sources: Coding guidelines, Learnings
| function isSlackProvider(provider: string): boolean { | ||
| const normalized = provider.trim().toLowerCase() | ||
| return normalized === 'slack' || normalized.startsWith('slack-') | ||
| } |
There was a problem hiding this comment.
Export isSlackProvider for reuse in other modules.
The context snippet shows isSlackProvider(provider) being called in integration-event-bridge.ts:397 as a gate for Slack-specific writeback behavior. Since this function is not exported, the consuming file either duplicates this logic or expects it to be exported from here. Given the PR's intent to "share Slack writeback command root grammar," this helper should be exported for consistent provider detection across modules.
📦 Proposed fix to export the function
-function isSlackProvider(provider: string): boolean {
+export function isSlackProvider(provider: string): boolean {
const normalized = provider.trim().toLowerCase()
return normalized === 'slack' || normalized.startsWith('slack-')
}📝 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.
| function isSlackProvider(provider: string): boolean { | |
| const normalized = provider.trim().toLowerCase() | |
| return normalized === 'slack' || normalized.startsWith('slack-') | |
| } | |
| export function isSlackProvider(provider: string): boolean { | |
| const normalized = provider.trim().toLowerCase() | |
| return normalized === 'slack' || normalized.startsWith('slack-') | |
| } |
🤖 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/slack-writeback-command-roots.ts` around lines 12 - 15, The helper
function isSlackProvider should be made available to other modules so they can
reuse the same provider-detection logic; update the declaration of
isSlackProvider to be a named export (export function isSlackProvider(...)) and
ensure any module that currently duplicates this logic (e.g.,
integration-event-bridge) imports this named export instead of reimplementing
it; keep the function signature and behavior unchanged to maintain
compatibility.
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Implemented and verified scoped fixes for PR #120. Changed:
Verification run:
I did not end with |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed the remaining validated review findings:
Verification run:
|
|
Addressed the remaining validated review findings:
Verification run:
|
|
Implemented the validated PR fixes and review feedback. Changed:
Verified locally:
|
|
✅ pr-reviewer applied fixes — committed and pushed Implemented the validated PR fixes and review feedback. Changed:
Verified locally:
|
|
Reviewed the PR diff and saved PR comments, validated the remaining findings against the current checkout, and fixed two still-applicable issues:
Verification passed locally:
|
|
Reviewed the PR diff and saved PR comments, validated the remaining findings against the current checkout, and fixed two still-applicable issues:
Verification passed locally:
|
|
Reviewed PR #120 against Local verification run:
All passed locally. |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed PR #120 against Local verification run:
All passed locally. |
1 participant
User description
Summary
Why
The local probe showed .integrations was linked and readable, but the live integration stream was repeatedly logging remote stream errors. Pear was resolving the Relayfile workspace token once before constructing RelayFileSync, so reconnects could keep retrying with a stale token after token rotation. That can make Slack/Nango forwards arrive upstream while Pear misses the live event stream until restart.
Tests
CodeAnt-AI Description
Use canonical Slack writeback command roots and keep integration streams alive after token refresh
What Changed
messagesandrepliesroots instead of.outboxImpact
✅ Fewer missed Slack events after token refresh✅ Clearer Slack writeback paths✅ No double-nested local integration mounts✅ Clearer remote stream failure messages💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.