Fix Relayfile mount packaging and active integration events#114
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. |
📝 WalkthroughWalkthroughThis PR introduces release-mode installation for the Relayfile mount binary. The build scripts now invoke a release-specific install task with an environment variable, the install script gains release-mode control flow and marker-based version tracking, the launcher imports a factory function from a new SDK subpath, and tests validate the entire integration. ChangesRelayfile Mount Release Mode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
codeant-ai
Bot
added
the
size:M
kjgbot
force-pushed
the
fix/relayfile-mount-release-launcher
branch
from
abf18e4 to
d3b3bb7
Compare
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| const optional = process.argv.includes('--optional') | ||
| const releaseMode = process.argv.includes('--release') || process.env.RELAYFILE_MOUNT_INSTALL_SOURCE === 'release' | ||
| const forceReleaseDownload = releaseMode && !optional |
There was a problem hiding this comment.
Suggestion: --release runs always force a download because forceReleaseDownload is true for non-optional installs, so the cached matching release binary is never reused. This causes unnecessary network dependence and can fail packaging even when the correct version is already installed; allow the same-version marker short-circuit for regular release installs too. [performance]
Severity Level: Major ⚠️
- ⚠️ Release installs always redownload relayfile-mount from GitHub.
- ⚠️ mac dist/release packaging depends on network every run.
- ⚠️ Offline or flaky-network release builds may fail unnecessarily.Steps of Reproduction ✅
1. From `/workspace/pear/package.json:14`, run `npm run relayfile-mount:install:release`,
which executes `node scripts/install-relayfile-mount.mjs --release` (non-optional,
releaseMode=true, optional=false).
2. On first run, `sdkVersion()` at `scripts/install-relayfile-mount.mjs:52-62` returns a
version, `downloadRelease(version)` at `scripts/install-relayfile-mount.mjs:91-110` runs
from inside the `if (releaseMode)` block at `scripts/install-relayfile-mount.mjs:124-135`,
and writes the version marker file via `writeFile(versionMarker, \`\${version}\n\`)` at
`scripts/install-relayfile-mount.mjs:109`.
3. Run `npm run relayfile-mount:install:release` again without deleting
`bin/relayfile-mount` or `.relayfile-mount-version`; `releaseMode` remains true and
`optional` false, so `forceReleaseDownload = releaseMode && !optional` at
`scripts/install-relayfile-mount.mjs:25` evaluates to `true`.
4. In the `if (releaseMode)` block at `scripts/install-relayfile-mount.mjs:124-135`,
`installedVersion` is correctly read by `installedMarker()` at
`scripts/install-relayfile-mount.mjs:118-122`, but the reuse check `if
(!forceReleaseDownload && installedVersion === version)` at line 126 is never satisfied
because `!forceReleaseDownload` is always `false`, so `downloadRelease(version)` is always
called again, forcing a fresh network download even when the installed version already
matches.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:** scripts/install-relayfile-mount.mjs
**Line:** 25:25
**Comment:**
*Performance: `--release` runs always force a download because `forceReleaseDownload` is true for non-optional installs, so the cached matching release binary is never reused. This causes unnecessary network dependence and can fail packaging even when the correct version is already installed; allow the same-version marker short-circuit for regular release installs too.
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| const version = sdkVersion() | ||
| if (!version) { | ||
| fail('could not determine @relayfile/sdk version (is it installed?)') |
There was a problem hiding this comment.
Suggestion: The installer now aborts when @relayfile/sdk is missing before it even checks RELAYFILE_MOUNT_BIN or local dist paths, which breaks the documented resolution order and prevents valid explicit/local installs from working. Resolve SDK version only right before the GitHub-release path (or only inside release mode), so explicit binary and local checkout installs do not depend on SDK package metadata. [logic error]
Severity Level: Major ⚠️
- ❌ `relayfile-mount:install` can fail before using explicit binary.
- ⚠️ Local relayfile dist path ignored if SDK version missing.
- ⚠️ Postinstall may skip valid local binaries when optional.Steps of Reproduction ✅
1. From `/workspace/pear/package.json:13`, run the dev install entrypoint `npm run
relayfile-mount:install` which executes `node scripts/install-relayfile-mount.mjs`.
2. Ensure `@relayfile/sdk` is not resolvable under `pearRoot/node_modules` (e.g.,
dependency not installed or only hoisted elsewhere), so `sdkVersion()` at
`scripts/install-relayfile-mount.mjs:52-62` returns `null`.
3. When the script runs, it evaluates `const version = sdkVersion()` at
`scripts/install-relayfile-mount.mjs:113` and immediately hits `if (!version) { fail(...)
}` at `scripts/install-relayfile-mount.mjs:114-115`, causing `fail()` at
`scripts/install-relayfile-mount.mjs:65-75` to exit before any other resolution paths run.
4. Because the process exits at step 3, the explicit binary branch (`RELAYFILE_MOUNT_BIN`
at `scripts/install-relayfile-mount.mjs:138-146`) and local dist branch (`localDist` at
`scripts/install-relayfile-mount.mjs:148-155`) are never reached, breaking the documented
resolution order in the header comments at `scripts/install-relayfile-mount.mjs:6-11`.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:** scripts/install-relayfile-mount.mjs
**Line:** 113:115
**Comment:**
*Logic Error: The installer now aborts when `@relayfile/sdk` is missing before it even checks `RELAYFILE_MOUNT_BIN` or local dist paths, which breaks the documented resolution order and prevents valid explicit/local installs from working. Resolve SDK version only right before the GitHub-release path (or only inside release mode), so explicit binary and local checkout installs do not depend on SDK package metadata.
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/main/__tests__/relayfile-mount-launcher-import.test.ts`:
- Around line 42-44: Tighten the test so it verifies downloadRelease(version) is
inside the releaseMode branch: update the assertion around installScript to
match the call only when it's within the if (releaseMode) { ... } block (for
example by using a single-regex that anchors the call to the if-block body, e.g.
matching "if (releaseMode) {" followed later by "await downloadRelease(version)"
with dot-all/s flag or an explicit block-content pattern), referencing the
existing installScript, releaseMode and downloadRelease symbols so the test
fails if the release branch stops invoking the downloader.
🪄 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: 5fa15562-d44a-4b1b-893a-1ee57f50687f
📒 Files selected for processing (4)
package.jsonscripts/install-relayfile-mount.mjssrc/main/__tests__/relayfile-mount-launcher-import.test.tssrc/main/relayfile-mount-launcher.ts
|
Reviewed PR #114 against the current checkout. I did not find a validated breakage that required edits. Validated locally:
I also tried a raw |
|
ℹ️ 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 #114 against the current checkout. I did not find a validated breakage that required edits. Validated locally:
I also tried a raw |
|
✅ pr-reviewer applied fixes — committed and pushed Fixed two validated review issues:
Validated locally:
I also checked PR bot feedback. The SDK-resolution comment was stale in the current checkout, so I did not change that path. One external Cubic review check was still |
|
Reviewed PR #114 and found no current-checkout breakage that needed edits. Validated:
Note: full |
|
ℹ️ 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 #114 and found no current-checkout breakage that needed edits. Validated:
Note: full |
kjgbot
changed the title
|
Reviewed PR #114 against the current checkout. I did not find a demonstrated PR-caused breakage that required edits. Verified:
I also tried |
|
ℹ️ 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 #114 against the current checkout. I did not find a demonstrated PR-caused breakage that required edits. Verified:
I also tried |
![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 5 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/integrations.ts">
<violation number="1" location="src/main/integrations.ts:1828">
P2: Inactive projects are marked as having ready subscriptions even after closing them, causing misleading integration system messages.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 9 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/ipc-handlers.ts">
<violation number="1" location="src/main/ipc-handlers.ts:128">
P2: Active-project persistence now happens before hydration, so a hydration error leaves the saved active project changed while the renderer treats the switch as failed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }) | ||
|
|
||
| ipcMain.handle('project:set-active', (_, id: string | null) => { | ||
| ipcMain.handle('project:set-active', async (_, id: string | null) => { |
There was a problem hiding this comment.
P2: Active-project persistence now happens before hydration, so a hydration error leaves the saved active project changed while the renderer treats the switch as failed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/ipc-handlers.ts, line 128:
<comment>Active-project persistence now happens before hydration, so a hydration error leaves the saved active project changed while the renderer treats the switch as failed.</comment>
<file context>
@@ -125,8 +125,9 @@ export function registerIpcHandlers(): void {
})
- ipcMain.handle('project:set-active', (_, id: string | null) => {
+ ipcMain.handle('project:set-active', async (_, id: string | null) => {
setActiveProject(id)
+ await integrationsManager.hydrateActiveProject(id)
</file context>
|
Fixed a real PR regression in src/main/integrations.ts: integration guidance is no longer marked as deduped until at least one agent is found and delivery completes. This prevents an empty first wait from suppressing the next valid notification. Added a regression test in src/main/integrations.test.ts covering the timeout-empty then retry-with-agent case. Validation run locally:
Note: an ad hoc |
|
✅ pr-reviewer applied fixes — committed and pushed Fixed a real PR regression in src/main/integrations.ts: integration guidance is no longer marked as deduped until at least one agent is found and delivery completes. This prevents an empty first wait from suppressing the next valid notification. Added a regression test in src/main/integrations.test.ts covering the timeout-empty then retry-with-agent case. Validation run locally:
Note: an ad hoc |
There was a problem hiding this comment.
2 issues found across 4 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/ipc-handlers.ts">
<violation number="1" location="src/main/ipc-handlers.ts:214">
P2: Spawn now injects integration guidance that can overstate subscription readiness, telling agents to wait for integration events before readiness is confirmed.</violation>
</file>
<file name="src/main/index.ts">
<violation number="1" location="src/main/index.ts:271">
P2: Switching post-ready handling from notify to refresh suppresses integration system messages for newly ready agents, which leaves persona spawns without integration context.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| ipcMain.handle('broker:spawn-agent', async (_, projectId: string, input: SpawnPtyInput & { broker?: 'local' | 'cloud' }) => { | ||
| const result = await brokerManager.spawnAgent(projectId, input) | ||
| const integrationInstructions = integrationsManager.initialSpawnInstructions(projectId) |
There was a problem hiding this comment.
P2: Spawn now injects integration guidance that can overstate subscription readiness, telling agents to wait for integration events before readiness is confirmed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/ipc-handlers.ts, line 214:
<comment>Spawn now injects integration guidance that can overstate subscription readiness, telling agents to wait for integration events before readiness is confirmed.</comment>
<file context>
@@ -211,7 +211,15 @@ export function registerIpcHandlers(): void {
ipcMain.handle('broker:spawn-agent', async (_, projectId: string, input: SpawnPtyInput & { broker?: 'local' | 'cloud' }) => {
- const result = await brokerManager.spawnAgent(projectId, input)
+ const integrationInstructions = integrationsManager.initialSpawnInstructions(projectId)
+ const result = await brokerManager.spawnAgent(projectId, integrationInstructions
+ ? {
</file context>
| integrationAgentRefreshTimers.delete(projectId) | ||
| void integrationsManager.notifyAgentState(projectId).catch((error) => { | ||
| console.warn('[integrations] Failed to notify newly ready agent:', error instanceof Error ? error.message : String(error)) | ||
| void integrationsManager.refreshAgentState(projectId).catch((error) => { |
There was a problem hiding this comment.
P2: Switching post-ready handling from notify to refresh suppresses integration system messages for newly ready agents, which leaves persona spawns without integration context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/index.ts, line 271:
<comment>Switching post-ready handling from notify to refresh suppresses integration system messages for newly ready agents, which leaves persona spawns without integration context.</comment>
<file context>
@@ -268,8 +268,8 @@ function scheduleIntegrationAgentRefresh(projectId: string): void {
integrationAgentRefreshTimers.delete(projectId)
- void integrationsManager.notifyAgentState(projectId).catch((error) => {
- console.warn('[integrations] Failed to notify newly ready agent:', error instanceof Error ? error.message : String(error))
+ void integrationsManager.refreshAgentState(projectId).catch((error) => {
+ console.warn('[integrations] Failed to refresh newly ready agent integration state:', error instanceof Error ? error.message : String(error))
})
</file context>
| void integrationsManager.refreshAgentState(projectId).catch((error) => { | |
| void integrationsManager.notifyAgentState(projectId).catch((error) => { |
1 participant
Summary
relayfile-mountfrom Pear release assets instead of relying on local SDK internals/importsValidation
npx vitest run src/main/integrations.test.tsnpm testnpm run build