fix(cli): negotiate relayfile control-plane API before v3 calls#1324
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughThe CLI pins ChangesRelayfile negotiation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c5e4c2cae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "@agent-relay/utils": "10.6.3", | ||
| "@modelcontextprotocol/sdk": "^1.0.0", | ||
| "@relayfile/client": "^0.10.21", | ||
| "@relayfile/client": "0.10.27-rc.0", |
There was a problem hiding this comment.
If this commit is merged and released as written, every agent-relay installation will remain pinned to the 0.10.27-rc.0 prerelease even after the stable client is published, preventing users from receiving the finalized artifact or subsequent compatible fixes. The commit's mandatory-before-merge note explicitly requires replacing this with ^0.10.27, so update both manifests and the lockfile before landing it.
Useful? React with 👍 / 👎.
![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 updates the @relayfile/client dependency, documents integration fixes in the changelog, and introduces comprehensive contract tests for the relayfile control-plane hello negotiation. The feedback suggests improving test maintainability by extracting large embedded JavaScript code strings from the test helpers into separate fixture files to enable proper syntax highlighting and linting.
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.
| response.end(JSON.stringify(body)); | ||
| } | ||
|
|
||
| async function withFakeRelayfileBinary( |
There was a problem hiding this comment.
The helper functions withFakeRelayfileBinary (lines 42-94) and withExternalControlPlaneDaemon (lines 96-188) embed significant amounts of JavaScript code as multi-line strings. This practice can make the code harder to read, debug, and maintain, as it lacks syntax highlighting and linting within the string literal.
For better maintainability, consider extracting these scripts into separate fixture files (e.g., in a __fixtures__ directory) and loading them with fs.readFileSync. This would improve code organization and developer experience.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/cli/src/cli/commands/integration-relayfile-contract.test.ts`:
- Around line 138-166: Add a short readiness timeout to the promise around the
fake daemon startup, rejecting with a clear error and killing the child when
“ready” is not emitted. Track the timer and clear it in cleanup(), while
preserving the existing success, error, and exit handling in the child readiness
flow.
- Around line 314-325: Update the integration tests around
defaultRelayfileBridge and withExternalControlPlaneDaemon to expose the daemon
child process to the callback, then assert the stale daemon has actually exited
after a compatible replacement. In the incompatibility case, assert the retained
daemon process remains alive, covering both replacement termination and
preservation lifecycle behavior.
- Around line 297-312: Make the auto-started daemon fixture in the “retries a
transient missing socket” test create its socket only after the initial request
attempt, while ensuring creation occurs before startTimeoutMs expires. Keep the
existing ensureCompatible assertion and cleanup unchanged so the test
deterministically exercises missing-socket retry 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: 2dc3a9bd-6f99-4098-b8fc-b01ebe305783
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
CHANGELOG.mdpackages/cli/package.jsonpackages/cli/src/cli/commands/integration-relayfile-contract.test.ts
| await new Promise<void>((resolve, reject) => { | ||
| let stderr = ''; | ||
| const fail = (message: string) => { | ||
| cleanup(); | ||
| reject(new Error(message)); | ||
| }; | ||
| const onData = (chunk: Buffer) => { | ||
| if (String(chunk).includes('ready')) { | ||
| cleanup(); | ||
| resolve(); | ||
| } | ||
| }; | ||
| const onError = (error: Error) => fail(`failed to start fake relayfile daemon: ${error.message}`); | ||
| const onExit = (code: number | null, signal: NodeJS.Signals | null) => | ||
| fail(`fake relayfile daemon exited before ready (code ${code}, signal ${signal}): ${stderr}`); | ||
| const cleanup = () => { | ||
| child.stdout?.off('data', onData); | ||
| child.stderr?.off('data', onStderr); | ||
| child.off('error', onError); | ||
| child.off('exit', onExit); | ||
| }; | ||
| const onStderr = (chunk: Buffer) => { | ||
| stderr += String(chunk); | ||
| }; | ||
| child.stdout?.on('data', onData); | ||
| child.stderr?.on('data', onStderr); | ||
| child.once('error', onError); | ||
| child.once('exit', onExit); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the fake daemon readiness wait.
If the child stays alive without emitting ready, this promise never settles and the suite hangs until its global timeout. Add a short timer that rejects and kills the child, clearing it from cleanup().
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { type ChildProcess, spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@packages/cli/src/cli/commands/integration-relayfile-contract.test.ts` around
lines 138 - 166, Add a short readiness timeout to the promise around the fake
daemon startup, rejecting with a clear error and killing the child when “ready”
is not emitted. Track the timer and clear it in cleanup(), while preserving the
existing success, error, and exit handling in the child readiness flow.
| it('retries a transient missing socket while an auto-started daemon becomes ready', async () => { | ||
| await withFakeRelayfileBinary('0.10.27', async (binary) => { | ||
| const socketPath = join(tmpdir(), `rf-transient-${process.pid}-${++socketSequence}.sock`); | ||
| rmSync(socketPath, { force: true }); | ||
| const bridge = defaultRelayfileBridge({ | ||
| socketPath, | ||
| binary, | ||
| autoStart: true, | ||
| startTimeoutMs: 2000, | ||
| requestTimeoutMs: 250, | ||
| }); | ||
|
|
||
| await expect(bridge.ensureCompatible()).resolves.toBeUndefined(); | ||
| rmSync(socketPath, { force: true }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the missing-socket retry deterministic.
The fixture may create its socket before the first request, allowing this test to pass without exercising a retry. Delay socket creation beyond the initial request attempt while keeping it within startTimeoutMs.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { type ChildProcess, spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@packages/cli/src/cli/commands/integration-relayfile-contract.test.ts` around
lines 297 - 312, Make the auto-started daemon fixture in the “retries a
transient missing socket” test create its socket only after the initial request
attempt, while ensuring creation occurs before startTimeoutMs expires. Keep the
existing ensureCompatible assertion and cleanup unchanged so the test
deterministically exercises missing-socket retry behavior.
| it('replaces a stale v1 daemon only when the installed binary can serve v3', async () => { | ||
| await withFakeRelayfileBinary('0.10.27', async (binary) => { | ||
| await withExternalControlPlaneDaemon('0.10.19', 1, [1], async (socketPath) => { | ||
| const bridge = defaultRelayfileBridge({ | ||
| socketPath, | ||
| binary, | ||
| autoStart: true, | ||
| startTimeoutMs: 2000, | ||
| requestTimeoutMs: 1000, | ||
| }); | ||
| await expect(bridge.ensureCompatible()).resolves.toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Assert the stale daemon’s actual process state.
These results do not prove the lifecycle contract: an unlink-only replacement or premature termination of the retained daemon could still pass. Expose the child to the callback, then assert it exited after replacement and remains alive after incompatibility.
Also applies to: 329-345
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { type ChildProcess, spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@packages/cli/src/cli/commands/integration-relayfile-contract.test.ts` around
lines 314 - 325, Update the integration tests around defaultRelayfileBridge and
withExternalControlPlaneDaemon to expose the daemon child process to the
callback, then assert the stale daemon has actually exited after a compatible
replacement. In the incompatibility case, assert the retained daemon process
remains alive, covering both replacement termination and preservation lifecycle
behavior.
E2E verification against published relayfile v0.10.27Ran the real Pin flip verified clean
(a) Cold start — no daemon running Daemon auto-started, (b) Stale daemon — the scenario relayfile#354 fixes Staged a genuine v0.10.19 daemon ( Stale v1-only daemon detected → stopped → replaced with 0.10.27 → renegotiated → v3 call succeeded. ✅ Direct Upstream: relayfile#354 merged ( Note: this exercised the macOS |
1 participant
Summary
@relayfile/client@^0.10.27final release/v1/hellodiscovery, fail-fast v1 incompatibility, transient socket retry, and stale-daemon replacementCounterpart: AgentWorkforce/relayfile#354
Final-package gate
@relayfile/client@0.10.27is published onlatest, replace the exact0.10.27-rc.0pin and lock entry with^0.10.27, then rerun validation. This relay PR must not merge with the RC pin.Validation
relayfile@latestand@relayfile/client@latestboth resolve0.10.270.10.27-rcreferences remain in tracked manifests or lockfilesnpm exec vitest run packages/cli/src/cli/commands/integration-relayfile-contract.test.ts— 10 passed, 4 opt-in skippednpm run typecheck— passednpm run lint— passed with 38 pre-existing warnings and 0 errorsnpx --yes npm@10.5.1 ci --dry-run --ignore-scripts— passed and resolves final client0.10.27npm ls @relayfile/client --all— resolves0.10.27The full repository Vitest run reached 1,255 passing and 11 skipped tests, then failed on the unrelated existing 5-second timeout in
broker-lifecycle.test.ts(shuts the broker down when its compiled-binary node provider exits). This branch does not modify that test or its implementation, and the failure reproduces when the file is run alone.