fix(slack): local-disk fallback for event preview + suffixed path emission#155
Conversation
…ssion readEventContextPreview() was SDK-read-only (client.readFile), which missed the suffixed mount candidate under remote-consistency lag even when the file was already on local disk. Events therefore emitted bare-path + "content unavailable" on a current build — #154 fixed candidate generation, not the read source. - Add local-disk fallback scoped to matched concrete mount roots only (no broad .integrations scan; preserves DM "watched-but-not-mounted" state) - Emit the resolved suffixed path for visible Path:, data.path, and data.resource.path (closes the structured-metadata-stays-bare gap) - Regression for bare event path + suffixed local mount + SDK read miss Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Warning Review limit reached
More reviews will be available in 51 minutes and 28 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 selected for processing (2)
📝 WalkthroughWalkthroughAdd guarded local filesystem fallback for Slack event context previews: compute concrete local/remote mount roots, attempt local reads for targeted context before remote fallback, compute a resolved targeted path, propagate it into message rendering and broker payloads, and cover behavior with two integration tests. ChangesLocal workspace mount fallback for Slack context reading
Sequence Diagram(s)sequenceDiagram
participant EventBridge
participant RemoteFS as Remote read/expand
participant LocalFS as Local filesystem
participant Injector
participant Broker
EventBridge->>RemoteFS: attempt remote context read (event.resource.path)
alt remote read fails
EventBridge->>LocalFS: compute resolvedSlackContextPath & try localMountRoots candidate reads
LocalFS-->>EventBridge: return local context preview (meta.json) or unavailable
else remote read succeeds
RemoteFS-->>EventBridge: return remote preview
end
EventBridge->>Injector: injectEvent with resolvedPath/resolvedResource
Injector->>Broker: publish payload with resource.path = resolvedPath
Possibly related PRs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![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 local fallback reading for event context previews when remote reads fail, and adds support for resolving suffixed Slack channel paths. A critical security review comment identified a potential directory traversal vulnerability in readLocalEventContextPreview where unvalidated path traversal segments could allow reading arbitrary files outside the intended local root; a fix was suggested to ensure the resolved path remains strictly within the local root directory.
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 (!pathIsInsideMount(remotePath, root.remoteRoot)) continue | ||
| const localPath = localPathForRemotePathInsideRoot(root.localRoot, root.remoteRoot, remotePath) | ||
| const stats = await stat(localPath).catch(() => null) | ||
| if (!stats || stats.isDirectory()) continue |
There was a problem hiding this comment.
The readLocalEventContextPreview function resolves a local path using localPathForRemotePathInsideRoot and reads the file directly. However, if the remotePath contains directory traversal segments (e.g., ..), it can escape the intended localRoot directory because pathIsInsideMount only performs a simple string prefix check on the unresolved path. This allows an attacker who can control the event resource path to read arbitrary files from the local disk.\n\nTo prevent this, validate that the resolved localPath is strictly within the resolved localRoot directory using relative before reading the file.
if (!pathIsInsideMount(remotePath, root.remoteRoot)) continue
const localPath = localPathForRemotePathInsideRoot(root.localRoot, root.remoteRoot, remotePath)
const relativePath = relative(resolve(root.localRoot), resolve(localPath))
if (!relativePath || relativePath.startsWith('..') || relativePath === '..') continue
const stats = await stat(localPath).catch(() => null)
if (!stats || stats.isDirectory()) continue
![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
🤖 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/__tests__/integration-event-bridge.test.ts`:
- Around line 878-945: The test needs a duplicate/replay emission to verify
deduping after the local-fallback path; after the first harness.emit(...) (using
changeEvent with remotePath) and waitForSent(...), emit a second event that
represents the replay/alias (e.g., same remotePath or the suffixed
localRemotePath with same digest) via harness.emit(...) and call waitForSent
again, then assert that the bridge did not create duplicate notifications (check
harness.sent length stays at 1 or that the second send is deduped) and that
harness.readFileCalls contains the expected remote then localRemotePath reads;
update assertions around harness.sent and harness.readFileCalls accordingly to
confirm the duplicate/replay branch behaves like the initial path.
In `@src/main/integration-event-bridge.ts`:
- Around line 990-992: The loop currently synthesizes local fallback roots for
every subscribed mountPath by calling
addRoot(localPathForRemoteRoot(workspaceId, mountPath), mountPath), which
enables readLocalEventContextPreview to treat unwatched-but-not-mounted paths as
concrete; change the loop to only add roots for mountPaths that correspond to
actual concrete local mounts (i.e., are present in integration.localMountPaths
or whichever concrete-mount collection is used), so only when
integration.localMountPaths includes mountPath call addRoot (leave
watched-but-not-mounted mountPaths alone); adjust the condition around
addRoot/localPathForRemoteRoot accordingly to reference
integration.localMountPaths, mountPaths, and
localMountRoot/readLocalEventContextPreview behavior.
🪄 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: 15e4d2fa-5da2-42e7-ac24-36aa2374b75a
📒 Files selected for processing (2)
src/main/__tests__/integration-event-bridge.test.tssrc/main/integration-event-bridge.ts
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/__tests__/integration-event-bridge.test.ts (1)
878-945:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a replay assertion for the local-fallback regression.
This proves the first local-disk fallback, but it still doesn't cover the second raw-id/slug replay after the initial inject. Please emit the alias/replay copy and assert
harness.sent.lengthstays at1so this branch is protected against duplicate regressions too.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/__tests__/integration-event-bridge.test.ts` around lines 878 - 945, The test currently verifies the local-disk fallback read but misses asserting that a subsequent alias/replay (raw-id/slug) injection does not create a duplicate notification; after the existing harness.emit(...) and waitForSent(...) calls, emit a second event representing the alias/replay (e.g., reuse changeEvent with path set to the localRemotePath or a raw-id/slug alias and the same digest) via harness.emit(...) and call waitForSent(...) as needed, then assert that harness.sent.length === 1 and that the single sent message still contains the expected localRemotePath in input.data and previews; update assertions around harness.sent to protect against duplicate deliveries (refer to the test name, harness.emit, changeEvent, waitForSent, and the harness.sent array).Source: Coding guidelines
src/main/integration-event-bridge.ts (1)
990-992:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't synthesize fallback roots from canonical mount paths.
This still populates
spec.localMountRootseven whenintegration.localMountPathsis empty, because every subscribedmountPathgets mapped into~/.agentworkforce/....readLocalEventContextPreview()will then treat watched-but-not-mounted paths as concrete local mounts again, which breaks the intended “no concrete mount → no fallback” behavior.Suggested fix
- for (const mountPath of mountPaths) { - addRoot(localPathForRemoteRoot(workspaceId, mountPath), mountPath) - } - return Array.from(roots.values())Based on learnings from the PR objectives, the fallback should stay limited to concrete local mounts and preserve watched-but-not-mounted behavior.
🤖 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/integration-event-bridge.ts` around lines 990 - 992, The code currently synthesizes fallback local roots by iterating mountPaths and calling addRoot(localPathForRemoteRoot(workspaceId, mountPath), mountPath), which populates spec.localMountRoots even when integration.localMountPaths is empty; change this so we only synthesize/add roots for concrete mounts: check integration.localMountPaths (or the equivalent field that lists actual mounted paths) and only run the addRoot loop when that array exists and is non-empty, otherwise skip adding any roots for mountPaths so watched-but-not-mounted entries are not treated as concrete; update the loop around addRoot/localPathForRemoteRoot to reference integration.localMountPaths (or guard with a length check) and leave readLocalEventContextPreview behavior unchanged.
🤖 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/integration-event-bridge.ts`:
- Around line 1072-1075: The localPathIsInsideRoot guard currently compares
resolved string paths but doesn’t dereference symlinks, allowing a symlink
inside localRoot to point outside; update the check used by
readLocalEventContextPreview to compare real filesystem locations (use
fs.realpath on both localRoot and localPath and verify the realPath of localPath
is inside the realPath of localRoot) and perform that realpath check before
calling stat/readFile, or alternatively reject symlinks by using fs.lstat on the
leaf (and optionally each path component) and failing if any lstat reports a
symlink; modify localPathIsInsideRoot (and its call site in
readLocalEventContextPreview) to implement one of these approaches.
---
Duplicate comments:
In `@src/main/__tests__/integration-event-bridge.test.ts`:
- Around line 878-945: The test currently verifies the local-disk fallback read
but misses asserting that a subsequent alias/replay (raw-id/slug) injection does
not create a duplicate notification; after the existing harness.emit(...) and
waitForSent(...) calls, emit a second event representing the alias/replay (e.g.,
reuse changeEvent with path set to the localRemotePath or a raw-id/slug alias
and the same digest) via harness.emit(...) and call waitForSent(...) as needed,
then assert that harness.sent.length === 1 and that the single sent message
still contains the expected localRemotePath in input.data and previews; update
assertions around harness.sent to protect against duplicate deliveries (refer to
the test name, harness.emit, changeEvent, waitForSent, and the harness.sent
array).
In `@src/main/integration-event-bridge.ts`:
- Around line 990-992: The code currently synthesizes fallback local roots by
iterating mountPaths and calling addRoot(localPathForRemoteRoot(workspaceId,
mountPath), mountPath), which populates spec.localMountRoots even when
integration.localMountPaths is empty; change this so we only synthesize/add
roots for concrete mounts: check integration.localMountPaths (or the equivalent
field that lists actual mounted paths) and only run the addRoot loop when that
array exists and is non-empty, otherwise skip adding any roots for mountPaths so
watched-but-not-mounted entries are not treated as concrete; update the loop
around addRoot/localPathForRemoteRoot to reference integration.localMountPaths
(or guard with a length check) and leave readLocalEventContextPreview behavior
unchanged.
🪄 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: 6c9ff573-48e0-4d80-9369-bf825b337aac
📒 Files selected for processing (2)
src/main/__tests__/integration-event-bridge.test.tssrc/main/integration-event-bridge.ts
| function localPathIsInsideRoot(localRoot: string, localPath: string): boolean { | ||
| const relativePath = relative(resolve(localRoot), resolve(localPath)) | ||
| return relativePath === '' || (!!relativePath && !relativePath.startsWith('..') && !isAbsolute(relativePath)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/integration-event-bridge.ts"
# Show around the reported lines
nl -ba "$FILE" | sed -n '1040,1105p' || true
echo "----"
nl -ba "$FILE" | sed -n '1760,1860p' || true
echo "----"
rg -n "localPathIsInsideRoot" "$FILE" || trueRepository: AgentWorkforce/pear
Length of output: 333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/integration-event-bridge.ts"
nl -ba "$FILE" | sed -n '1040,1105p' || true
echo "----"
nl -ba "$FILE" | sed -n '1760,1860p' || true
echo "----"
rg -n "localPathIsInsideRoot" "$FILE" || trueRepository: AgentWorkforce/pear
Length of output: 333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/integration-event-bridge.ts"
sed -n '1055,1105p' "$FILE" | cat
echo "----"
sed -n '1785,1865p' "$FILE" | cat
echo "----"
# show nearby fs usage around call site
rg -n "stat\\(|readFile\\(|fs\\.stat\\(|fs\\.readFile\\(" "$FILE" | rg -n "18[0-9]{2}|185|186|1828" || trueRepository: AgentWorkforce/pear
Length of output: 5337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/main/integration-event-bridge.ts"
# imports near top
sed -n '1,80p' "$FILE" | cat
echo "----"
# confirm all usages in repo
rg -n "localPathIsInsideRoot\\(" -S . || trueRepository: AgentWorkforce/pear
Length of output: 3254
🌐 Web query:
Does Node.js fs.promises.stat follow symlinks (vs fs.lstat)? Does fs.promises.readFile follow symlinks?
💡 Result:
In Node.js, the behavior regarding symbolic links for these file system methods is as follows: fs.promises.stat vs. fs.promises.lstat fs.promises.stat follows symbolic links [1][2]. If the path provided refers to a symbolic link, it returns the stats of the target file or directory that the link points to [2]. fs.promises.lstat does not follow symbolic links [2][3]. It returns the stats of the symbolic link itself, rather than the file or directory it refers to [2][3]. fs.promises.readFile fs.promises.readFile follows symbolic links [4]. When you use fs.promises.readFile on a path that is a symbolic link, it reads the contents of the target file that the link points to, rather than the link itself [4]. If the path does not point to a valid file (e.g., a broken symlink), the operation will fail.
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: https://nodejs.org/docs/v22.14.0/api/fs.html
- 3: https://nodejs.org/docs/latest-v24.x/api/fs.html
- 4: https://stackoverflow.com/questions/51896873/how-to-read-a-symlink-in-node-js
Fix symlink escape in local-root guard (stat/readFile follow symlinks)
localPathIsInsideRoot (around src/main/integration-event-bridge.ts:1072) only compares resolve()d string paths; it doesn’t dereference symlinks. In readLocalEventContextPreview, the guard at ~1828 is followed by stat(localPath) and readFile(localPath) (both follow symlinks), so a symlink under root.localRoot can point outside the real root and still be read/used for the preview.
Suggested fix
- Make the check compare real filesystem locations (e.g.,
realpath(localRoot)vsrealpath(localPath)), and run it beforestat()/readFile(). - Alternatively/reinforcing: reject symlinks via
lstaton the leaf and on each path component before reading.
🤖 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/integration-event-bridge.ts` around lines 1072 - 1075, The
localPathIsInsideRoot guard currently compares resolved string paths but doesn’t
dereference symlinks, allowing a symlink inside localRoot to point outside;
update the check used by readLocalEventContextPreview to compare real filesystem
locations (use fs.realpath on both localRoot and localPath and verify the
realPath of localPath is inside the realPath of localRoot) and perform that
realpath check before calling stat/readFile, or alternatively reject symlinks by
using fs.lstat on the leaf (and optionally each path component) and failing if
any lstat reports a symlink; modify localPathIsInsideRoot (and its call site in
readLocalEventContextPreview) to implement one of these approaches.
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
![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 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/integration-event-bridge.ts">
<violation number="1" location="src/main/integration-event-bridge.ts:1070">
P2: `localPathIsInsideRoot` misclassifies valid in-root paths whose first segment starts with `..` (e.g. `..meta.json`) as traversal attempts.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| function localPathIsInsideRoot(localRoot: string, localPath: string): boolean { | ||
| const relativePath = relative(resolve(localRoot), resolve(localPath)) | ||
| return relativePath === '' || (!!relativePath && !relativePath.startsWith('..') && !isAbsolute(relativePath)) |
There was a problem hiding this comment.
P2: localPathIsInsideRoot misclassifies valid in-root paths whose first segment starts with .. (e.g. ..meta.json) as traversal attempts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/integration-event-bridge.ts, line 1070:
<comment>`localPathIsInsideRoot` misclassifies valid in-root paths whose first segment starts with `..` (e.g. `..meta.json`) as traversal attempts.</comment>
<file context>
@@ -1069,6 +1065,11 @@ function localPathForRemotePathInsideRoot(localRoot: string, remoteRoot: string,
+function localPathIsInsideRoot(localRoot: string, localPath: string): boolean {
+ const relativePath = relative(resolve(localRoot), resolve(localPath))
+ return relativePath === '' || (!!relativePath && !relativePath.startsWith('..') && !isAbsolute(relativePath))
+}
+
</file context>
| return relativePath === '' || (!!relativePath && !relativePath.startsWith('..') && !isAbsolute(relativePath)) | |
| return relativePath === '' || (!!relativePath && relativePath !== '..' && !relativePath.startsWith(`..${sep}`) && !isAbsolute(relativePath)) |
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/integration-event-bridge.ts">
<violation number="1" location="src/main/integration-event-bridge.ts:1825">
P2: Local fallback mount-boundary check can be bypassed via symlinks, allowing reads outside the intended local root.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!pathIsInsideMount(remotePath, root.remoteRoot)) continue | ||
| const localPath = localPathForRemotePathInsideRoot(root.localRoot, root.remoteRoot, remotePath) | ||
| if (!localPathIsInsideRoot(root.localRoot, localPath)) continue | ||
| const stats = await stat(localPath).catch(() => null) |
There was a problem hiding this comment.
P2: Local fallback mount-boundary check can be bypassed via symlinks, allowing reads outside the intended local root.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/integration-event-bridge.ts, line 1825:
<comment>Local fallback mount-boundary check can be bypassed via symlinks, allowing reads outside the intended local root.</comment>
<file context>
@@ -1746,6 +1791,56 @@ function slackContextReadCandidatePaths(path: string, specs: SubscriptionSpec[])
+ if (!pathIsInsideMount(remotePath, root.remoteRoot)) continue
+ const localPath = localPathForRemotePathInsideRoot(root.localRoot, root.remoteRoot, remotePath)
+ if (!localPathIsInsideRoot(root.localRoot, localPath)) continue
+ const stats = await stat(localPath).catch(() => null)
+ if (!stats || stats.isDirectory()) continue
+ try {
</file context>
1 participant
Problem
Slack
<integration-event>notifications were emitting bare-channel-id paths withMessage: unavailable; targeted context read did not return content— even on a current build that already includes the #154 suffixed-candidate fix.Root cause:
readEventContextPreview()was SDK-read-only (client.readFile). The bare→suffixed candidate translation (#154) was correct, but the Relayfile SDK read still missed the suffixed candidate under remote-consistency lag while the bytes were already present on local disk. #154 fixed candidate generation, not the read source. The symptom fires even on the bot's own outbound messages echoing back, and fully blocks any worktree with no mount.Fix
readEventContextPreview(), tried after SDKreadFilecandidates and beforeevent.expand('full'). Scoped to matched concrete mount roots only (concreteLocalMountRootsForIntegration+pathIsInsideMount) — no broad.integrationsscan, and it preserves the DM "watched-but-not-mounted" state (no concrete mount → no fallback).Path:,data.path, anddata.resource.path— closes the structured-metadata-stays-bare gap so unmounted/downstream consumers also get the usable path.Tests
integration-event-bridge.test.ts: 63/63 pass.Review
Code-approved by the investigation team (diagnosed + verified against agreed constraints: concrete-mount-scoped fallback, suffixed path into all three fields, DM safety preserved).
Not in scope (tracked separately)
listenDmsis watch-only today)🤖 Generated with Claude Code