feat(cli): drive relayfile integration ops over the control-plane socket (no shell-out)#1215
Conversation
…ket (no shell-out)
Replace the stringly-typed `spawn('relayfile', …)` + parse-stdout bridge with a
typed client over relayfile's control-plane unix socket (`relayfile
control-plane serve`). The contract is now version-negotiated via `/v1/hello`,
and request/response types are generated from relayfile's OpenAPI — so field
drift is a compile error, not a runtime surprise.
- New `relayfile-client.ts`: HTTP/JSON over `http.request({ socketPath })`,
`X-Relayfile-API-Version` handshake, daemon auto-start (+ RELAYFILE_REQUIRE_DAEMON
strict mode), typed methods aliasing the generated schemas.
- `defaultRelayfileBridge` swapped onto the client; `runRelayfile`/`spawn`
deleted. The `RelayfileBridge` interface is unchanged, so subscribe/unsubscribe
callers are untouched.
- subscribe/unsubscribe now canonicalize the native `--resource` to relayfile's
stored path-glob via `resolve-path` before keying find/unbind (fixes the
silent native-vs-glob mismatch).
- Types generated from a vendored copy of relayfile's
openapi/relayfile-control-plane-v1.openapi.yaml (`npm run codegen:relayfile`).
This is the interim home; it will be swapped to the published `@relayfile/client`
package before merge once relayfile 0.10.16 ships.
- Tests: real-daemon contract test (boots the daemon, drives the bridge over the
socket) + client lifecycle units. Requires relayfile >= 0.10.16.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a Relayfile control-plane OpenAPI contract and generated types, implements a typed unix-socket client, and updates integration subscribe/unsubscribe to resolve resources to canonical ChangesRelayfile control-plane client and integration update
Sequence Diagram(s)sequenceDiagram
participant integration.ts
participant RelayfileControlPlaneClient
participant relayfile control-plane
integration.ts->>RelayfileControlPlaneClient: ensureCompatible()
RelayfileControlPlaneClient->>relayfile control-plane: GET /v1/hello
relayfile control-plane-->>RelayfileControlPlaneClient: daemonVersion, supportedApiVersions
RelayfileControlPlaneClient->>RelayfileControlPlaneClient: assertRelayfileVersion()
integration.ts->>RelayfileControlPlaneClient: resolvePath(provider, resource)
RelayfileControlPlaneClient->>relayfile control-plane: POST /v1/integrations/resolve-path
relayfile control-plane-->>RelayfileControlPlaneClient: pathGlob, warning?
integration.ts->>RelayfileControlPlaneClient: bind / listBindings / unbind
RelayfileControlPlaneClient->>relayfile control-plane: HTTP over unix socket
relayfile control-plane-->>RelayfileControlPlaneClient: typed response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 replaces the legacy spawn('relayfile') and stdout-parsing approach with a typed client (RelayfileControlPlaneClient) that communicates with the relayfile control-plane daemon over a local Unix domain socket. It introduces generated TypeScript types from the OpenAPI specification, updates CLI commands to use the new client, and adds comprehensive contract and lifecycle tests. The review feedback highlights three important reliability improvements in relayfile-client.ts: handling asynchronous 'error' events on the spawned daemon process to prevent CLI crashes, providing default values in compareSemver to handle partial semver strings robustly, and listening to 'error' events on the HTTP response stream.
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.
| private async startDaemonAndConnect(): Promise<HelloResponse> { | ||
| let child; | ||
| try { | ||
| child = spawn(this.binary, ['control-plane', 'serve', '--sock', this.socketPath], { | ||
| detached: true, | ||
| stdio: 'ignore', | ||
| }); | ||
| } catch (err) { | ||
| throw new RelayfileControlPlaneError( | ||
| 'DAEMON_UNAVAILABLE', | ||
| `failed to start relayfile control-plane (${err instanceof Error ? err.message : String(err)}). ` + | ||
| `Install relayfile or set RELAYFILE_BIN.` | ||
| ); | ||
| } | ||
| child.unref?.(); | ||
| const deadline = Date.now() + this.startTimeoutMs; | ||
| let lastErr: unknown; | ||
| while (Date.now() < deadline) { | ||
| await sleep(100); | ||
| try { | ||
| return await this.hello(); | ||
| } catch (err) { | ||
| lastErr = err; | ||
| } | ||
| } | ||
| throw new RelayfileControlPlaneError( | ||
| 'DAEMON_UNAVAILABLE', | ||
| `relayfile control-plane did not become ready within ${this.startTimeoutMs}ms ` + | ||
| `(${lastErr instanceof Error ? lastErr.message : String(lastErr)}).` | ||
| ); | ||
| } |
There was a problem hiding this comment.
When spawning the daemon process via spawn, if the binary is missing or fails to execute, Node.js will emit an 'error' event asynchronously on the returned ChildProcess object. Because there is no 'error' event listener registered on child, this will trigger an uncaught exception and crash the entire CLI process.
Adding an 'error' event listener to the spawned child process prevents this crash and allows the client to fail fast with a descriptive error instead of waiting for the connection timeout.
private async startDaemonAndConnect(): Promise<HelloResponse> {
let child;
let spawnError: Error | undefined;
try {
child = spawn(this.binary, ['control-plane', 'serve', '--sock', this.socketPath], {
detached: true,
stdio: 'ignore',
});
child.on('error', (err) => {
spawnError = err;
});
} catch (err) {
throw new RelayfileControlPlaneError(
'DAEMON_UNAVAILABLE',
`failed to start relayfile control-plane (${err instanceof Error ? err.message : String(err)}). ` +
`Install relayfile or set RELAYFILE_BIN.`
);
}
child.unref?.();
const deadline = Date.now() + this.startTimeoutMs;
let lastErr: unknown;
while (Date.now() < deadline) {
if (spawnError) {
throw new RelayfileControlPlaneError(
'DAEMON_UNAVAILABLE',
`failed to start relayfile control-plane (${spawnError.message}). ` +
`Install relayfile or set RELAYFILE_BIN.`
);
}
await sleep(100);
try {
return await this.hello();
} catch (err) {
lastErr = err;
}
}
throw new RelayfileControlPlaneError(
'DAEMON_UNAVAILABLE',
`relayfile control-plane did not become ready within ${this.startTimeoutMs}ms ` +
`(${lastErr instanceof Error ? lastErr.message : String(lastErr)}).`
);
}| const [a0, a1, a2] = core(a); | ||
| const [b0, b1, b2] = core(b); | ||
| return (a0! - b0!) || (a1! - b1!) || (a2! - b2!); |
There was a problem hiding this comment.
In compareSemver, if either version string a or b is missing minor or patch components (e.g., "0.10"), destructuring core(a) will result in undefined for a2. Subtracting undefined from a number results in NaN, causing the function to return NaN instead of a valid comparison integer.
Using default values during destructuring resolves this issue and ensures robust comparison even for partial semver strings.
| const [a0, a1, a2] = core(a); | |
| const [b0, b1, b2] = core(b); | |
| return (a0! - b0!) || (a1! - b1!) || (a2! - b2!); | |
| const [a0 = 0, a1 = 0, a2 = 0] = core(a); | |
| const [b0 = 0, b1 = 0, b2 = 0] = core(b); | |
| return (a0 - b0) || (a1 - b1) || (a2 - b2); |
| (res) => { | ||
| const chunks: Buffer[] = []; |
There was a problem hiding this comment.
The IncomingMessage object (res) is a readable stream and can emit an 'error' event. If an error occurs on the response stream and there is no listener, it can lead to unhandled rejections or crashes. Registering a listener that rejects the promise ensures robust error handling.
| (res) => { | |
| const chunks: Buffer[] = []; | |
| (res) => { | |
| res.on('error', reject); | |
| const chunks: Buffer[] = []; |
|
Depends on / pairs with AgentWorkforce/relayfile#344 (control-plane daemon + OpenAPI + Merge order: relayfile#344 merges and publishes |
![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: f0f089e8ea
ℹ️ 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".
| child = spawn(this.binary, ['control-plane', 'serve', '--sock', this.socketPath], { | ||
| detached: true, | ||
| stdio: 'ignore', | ||
| }); |
There was a problem hiding this comment.
When the socket is absent and relayfile is not installed (or RELAYFILE_BIN points to a bad path), spawn() reports ENOENT asynchronously on the returned child rather than throwing here. Because this child has no error listener, the process gets an unhandled error event instead of rejecting with the intended DAEMON_UNAVAILABLE message, so fresh installs/bad binary configs crash during any integration command that auto-starts the daemon.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!parsed || compareSemver(parsed, MIN_RELAYFILE_VERSION) < 0) { | ||
| throw new Error( | ||
| `relayfile >= ${MIN_RELAYFILE_VERSION} is required; found ${ |
There was a problem hiding this comment.
🟡 Version-too-old error silently swallowed during writeback secret resolution
The daemon version gate throws a plain Error (throw new Error(...) at packages/cli/src/cli/lib/relayfile-client.ts:56), but the writeback handler only re-throws RelayfileControlPlaneError, so the version failure is silently swallowed and the caller receives undefined instead of an actionable error.
Impact: A too-old daemon's version error could be masked, causing a misleading "Ensure relayfile is logged in" message instead of the intended "update relayfile" prompt.
Error type mismatch between assertRelayfileVersion and resolveWritebackBinding catch clause
In connectAndNegotiate (relayfile-client.ts:232-256), two version checks run:
- Line 247–253: API version check → throws
new RelayfileControlPlaneError('VERSION_INCOMPATIBLE', ...) - Line 255:
assertRelayfileVersion(hello.daemonVersion)→ throws plainnew Error(...)
In resolveWritebackBinding (integration.ts:219-224), the catch clause is:
if (err instanceof RelayfileControlPlaneError && err.code === 'VERSION_INCOMPATIBLE') throw err;
return undefined;
The plain Error from check #2 fails the instanceof test and is swallowed, returning undefined. The comment on lines 220-221 explicitly states "a daemon/version failure is not [a soft miss]", but this version failure IS silently swallowed.
In the current flow, ensureCompatible() is always called before resolveWritebackBinding (integration.ts:584, integration.ts:691), and ensureReady() caches its result, so the version check in connectAndNegotiate runs only once — meaning this path is unreachable with current callers. However, the error type inconsistency violates the code's stated contract.
| if (!parsed || compareSemver(parsed, MIN_RELAYFILE_VERSION) < 0) { | |
| throw new Error( | |
| `relayfile >= ${MIN_RELAYFILE_VERSION} is required; found ${ | |
| if (!parsed || compareSemver(parsed, MIN_RELAYFILE_VERSION) < 0) { | |
| throw new RelayfileControlPlaneError( | |
| 'VERSION_INCOMPATIBLE', |
Was this helpful? React with 👍 or 👎 to provide feedback.
| * under RELAYFILE_REQUIRE_DAEMON=1). The wire contract is version-negotiated, so | ||
| * field/method drift is a typed error, not a silent runtime surprise. | ||
| */ | ||
| export function defaultRelayfileBridge(options?: RelayfileClientOptions): RelayfileBridge { |
There was a problem hiding this comment.
🟡 Changelog not updated for user-visible control-plane migration
No CHANGELOG.md entry was added for this PR's user-visible changes (the [Unreleased] section is unchanged), violating the AGENTS.md rule "Curate [Unreleased] in CHANGELOG.md as you land PRs."
Impact: The release narrative omits significant behavior changes (daemon auto-start, version negotiation, resource path resolution) that users and operators need to know about.
Missing changelog entries for control-plane socket migration
AGENTS.md requires: "Curate [Unreleased] in CHANGELOG.md as you land PRs. The root changelog is the cross-package, user-facing release narrative for Relay."
This PR introduces several user-visible changes:
integration subscribeandunsubscribenow talk to a control-plane unix socket daemon instead of spawningrelayfileper operation — changing error messages, startup behavior (daemon auto-start), and adding a version-negotiation handshake.- Resource identifiers (e.g.
owner/repo,#channel) are now automatically resolved to canonical VFS path globs before binding. - A new compatibility check (
ensureCompatible) gates operations on daemon version, producing actionable "update relayfile" errors.
None of these are documented in the [Unreleased] section of CHANGELOG.md.
Prompt for agents
The AGENTS.md rule requires updating the [Unreleased] section in CHANGELOG.md when landing PRs. This PR introduces several user-visible changes that need changelog entries:
1. integration subscribe/unsubscribe now uses a control-plane unix socket (relayfile control-plane serve) instead of spawning relayfile per operation, with auto-start and version negotiation.
2. Resource identifiers (e.g. owner/repo, #channel) are now resolved to canonical VFS path globs via the daemon's resolve-path endpoint before binding/matching.
3. A compatibility check (ensureCompatible) gates operations on daemon version, producing actionable upgrade errors.
Add one or two concise bullets under Changed in the [Unreleased] section of CHANGELOG.md, following the existing style: name the command touched and the practical effect. For example under Changed: integration subscribe/unsubscribe now communicates with the relayfile daemon over a local control-plane socket (auto-started on first use) instead of spawning one process per operation, and canonicalizes provider-native resource identifiers to the VFS glob relayfile stores bindings under.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async connect(provider) { | ||
| await runRelayfile(['integration', 'connect', provider], { inherit: true }); | ||
| // OAuth runs in the daemon; on a local daemon the browser still opens | ||
| // (noOpen defaults false). The returned output carries any connect URL. | ||
| const { output } = await client.connect({ provider }); | ||
| if (output?.trim()) process.stderr.write(output.endsWith('\n') ? output : `${output}\n`); |
There was a problem hiding this comment.
🚩 Behavioral change: connect output is buffered instead of streamed
The old connect method used spawn('relayfile', ..., { inherit: true }) which pipes daemon stdout/stderr to the terminal in real time. The new implementation (packages/cli/src/cli/commands/integration.ts:172-173) captures the response body's output field and writes it to stderr only after the HTTP response completes. For OAuth flows that take several seconds (waiting for browser callback), the user sees no progress output until the flow finishes. This is a UX regression if the daemon's connect endpoint produces incremental status messages, but acceptable if the daemon simply opens a browser and waits silently.
Was this helpful? React with 👍 or 👎 to provide feedback.
Review SummaryPR #1215 replaces the legacy Verification performed (the way CI runs it)
Mechanical fixes auto-applied (non-semantic)
Not changed (left as advisory / human judgment)
Advisory Notes
Addressed comments
Note: CI status could not be observed from this sandbox (no |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/cli/src/cli/commands/integration-subscribe.test.ts (1)
82-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one non-identity resolve-path case here.
This stub always returns
pathGlob === resource, so these tests would still pass ifrunSubscribe()/runUnsubscribe()accidentally kept using the native string after resolution. One case that mapsowner/repoto/github/repos/owner/repo/**and assertsbind/unbinduse the glob would lock in the main regression this PR is fixing.🤖 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-subscribe.test.ts` around lines 82 - 85, Add a non-identity resolve-path test case in integration-subscribe.test.ts so the mock for resolveResourcePath no longer always returns the input unchanged. Extend the runSubscribe/runUnsubscribe coverage to include a provider/resource pair like owner/repo that resolves to /github/repos/owner/repo/**, and assert the bind/unbind calls use the resolved pathGlob rather than the original string. Use the existing runSubscribe, runUnsubscribe, bind, unbind, and resolveResourcePath symbols to place the new assertion alongside the current stubbed setup.
🤖 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.ts`:
- Around line 214-223: The catch in resolveWritebackBinding is swallowing daemon
outages by returning undefined for DAEMON_UNAVAILABLE and
non-RelayfileControlPlaneError failures, which makes resolveWriteback() report
the wrong remediation. Update the error handling in resolveWritebackBinding to
only treat true missing/disconnected writeback secrets as a soft miss, while
re-throwing daemon/control-plane outage errors from
client.writebackSecret(channel), alongside VERSION_INCOMPATIBLE; keep the
undefined fallback only for expected absent-secret cases.
In `@packages/cli/src/cli/lib/relayfile-client.test.ts`:
- Around line 103-106: The teardown in the relayfile client test uses
spawn('pkill', ...) without handling the case where pkill is missing, which can
crash the suite on some runners. Update the afterEach cleanup in
relayfile-client.test.ts to either use spawnSync for the pkill call, or add an
error handler/platform guard around the existing spawn invocation so missing
pkill does not produce an unhandled child process error. Keep the rest of the
cleanup logic for strays unchanged.
In `@packages/cli/src/cli/lib/relayfile-client.ts`:
- Around line 155-216: Add an explicit timeout to the Unix-socket request flow
in relayfile-client’s request helper so a silent daemon cannot leave the call
hanging indefinitely. Update the http.request-based logic to enforce a
per-request timeout for the path used by startDaemonAndConnect()/v1/hello, and
make sure the timeout rejects with RelayfileControlPlaneError using a clear
daemon-unavailable message. Keep the existing error handling in request() for
response parsing and socket errors, but ensure the new timeout is wired into the
same promise lifecycle.
- Around line 258-272: The daemon startup flow in startDaemonAndConnect() is not
handling spawn failures correctly, and rawRequest() has no timeout. Move the
DAEMON_UNAVAILABLE mapping to the child process’s error handling for spawn() so
ENOENT/bad RELAYFILE_BIN failures from the child’s error event are caught and
surfaced instead of crashing the CLI, and add a timeout or abort path around the
/v1/hello request so a hung hello call cannot wait forever. Use the existing
startDaemonAndConnect(), rawRequest(), and RelayfileControlPlaneError symbols to
wire the fix in the same control-plane client flow.
In
`@packages/cli/src/cli/lib/relayfile-contract/relayfile-control-plane-v1.openapi.yaml`:
- Around line 18-19: The /v1/hello operation is modeled with query/body
versioning only, but the client in relayfile-client.ts actually negotiates via
X-Relayfile-API-Version. Update the hello operation in
relayfile-control-plane-v1.openapi.yaml to include ApiVersionHeader alongside
the existing version parameter setup, and only keep ApiVersionQuery/body if the
daemon truly supports those shapes. Use the existing ApiVersionHeader and
ApiVersionQuery symbols to locate the affected operation and make the contract
match the real handshake.
---
Nitpick comments:
In `@packages/cli/src/cli/commands/integration-subscribe.test.ts`:
- Around line 82-85: Add a non-identity resolve-path test case in
integration-subscribe.test.ts so the mock for resolveResourcePath no longer
always returns the input unchanged. Extend the runSubscribe/runUnsubscribe
coverage to include a provider/resource pair like owner/repo that resolves to
/github/repos/owner/repo/**, and assert the bind/unbind calls use the resolved
pathGlob rather than the original string. Use the existing runSubscribe,
runUnsubscribe, bind, unbind, and resolveResourcePath symbols to place the new
assertion alongside the current stubbed setup.
🪄 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: d1a0ddff-252f-41fd-95c7-5ee4e1d68e0c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonpackages/cli/.eslintrc.cjspackages/cli/src/cli/commands/integration-relayfile-contract.test.tspackages/cli/src/cli/commands/integration-subscribe.test.tspackages/cli/src/cli/commands/integration.tspackages/cli/src/cli/commands/relaycast-groups.test.tspackages/cli/src/cli/lib/relayfile-client.test.tspackages/cli/src/cli/lib/relayfile-client.tspackages/cli/src/cli/lib/relayfile-contract/relayfile-control-plane-v1.gen.tspackages/cli/src/cli/lib/relayfile-contract/relayfile-control-plane-v1.openapi.yaml
| async resolveWritebackBinding(channel) { | ||
| try { | ||
| const output = await runRelayfile([ | ||
| 'integration', | ||
| 'writeback-secret', | ||
| '--channel', | ||
| channel, | ||
| '--json', | ||
| ]); | ||
| const parsed = JSON.parse(output) as { url?: unknown; secret?: unknown }; | ||
| const url = typeof parsed.url === 'string' ? parsed.url.trim() : ''; | ||
| const secret = typeof parsed.secret === 'string' ? parsed.secret.trim() : ''; | ||
| if (!url || !secret) { | ||
| return undefined; | ||
| } | ||
| return { url, secret }; | ||
| } catch { | ||
| const { url, secret } = await client.writebackSecret(channel); | ||
| if (!url?.trim() || !secret?.trim()) return undefined; | ||
| return { url: url.trim(), secret: secret.trim() }; | ||
| } catch (err) { | ||
| // A missing/disconnected writeback secret is a soft miss (caller falls | ||
| // back to --bridge-url/--bridge-secret); a daemon/version failure is not. | ||
| if (err instanceof RelayfileControlPlaneError && err.code === 'VERSION_INCOMPATIBLE') throw err; | ||
| return undefined; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Re-throw daemon outages instead of treating them like a missing secret.
This catch currently converts DAEMON_UNAVAILABLE and any non-RelayfileControlPlaneError into undefined, so resolveWriteback() reports a login/flag remediation even when the control-plane died mid-command. That hides the real failure mode and contradicts the comment above this block.
Suggested fix
} catch (err) {
// A missing/disconnected writeback secret is a soft miss (caller falls
// back to --bridge-url/--bridge-secret); a daemon/version failure is not.
- if (err instanceof RelayfileControlPlaneError && err.code === 'VERSION_INCOMPATIBLE') throw err;
+ if (!(err instanceof RelayfileControlPlaneError)) throw err;
+ if (err.code === 'VERSION_INCOMPATIBLE' || err.code === 'DAEMON_UNAVAILABLE') throw err;
return undefined;
}📝 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.
| async resolveWritebackBinding(channel) { | |
| try { | |
| const output = await runRelayfile([ | |
| 'integration', | |
| 'writeback-secret', | |
| '--channel', | |
| channel, | |
| '--json', | |
| ]); | |
| const parsed = JSON.parse(output) as { url?: unknown; secret?: unknown }; | |
| const url = typeof parsed.url === 'string' ? parsed.url.trim() : ''; | |
| const secret = typeof parsed.secret === 'string' ? parsed.secret.trim() : ''; | |
| if (!url || !secret) { | |
| return undefined; | |
| } | |
| return { url, secret }; | |
| } catch { | |
| const { url, secret } = await client.writebackSecret(channel); | |
| if (!url?.trim() || !secret?.trim()) return undefined; | |
| return { url: url.trim(), secret: secret.trim() }; | |
| } catch (err) { | |
| // A missing/disconnected writeback secret is a soft miss (caller falls | |
| // back to --bridge-url/--bridge-secret); a daemon/version failure is not. | |
| if (err instanceof RelayfileControlPlaneError && err.code === 'VERSION_INCOMPATIBLE') throw err; | |
| return undefined; | |
| async resolveWritebackBinding(channel) { | |
| try { | |
| const { url, secret } = await client.writebackSecret(channel); | |
| if (!url?.trim() || !secret?.trim()) return undefined; | |
| return { url: url.trim(), secret: secret.trim() }; | |
| } catch (err) { | |
| // A missing/disconnected writeback secret is a soft miss (caller falls | |
| // back to --bridge-url/--bridge-secret); a daemon/version failure is not. | |
| if (!(err instanceof RelayfileControlPlaneError)) throw err; | |
| if (err.code === 'VERSION_INCOMPATIBLE' || err.code === 'DAEMON_UNAVAILABLE') throw err; | |
| return undefined; | |
| } |
🤖 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.ts` around lines 214 - 223, The
catch in resolveWritebackBinding is swallowing daemon outages by returning
undefined for DAEMON_UNAVAILABLE and non-RelayfileControlPlaneError failures,
which makes resolveWriteback() report the wrong remediation. Update the error
handling in resolveWritebackBinding to only treat true missing/disconnected
writeback secrets as a soft miss, while re-throwing daemon/control-plane outage
errors from client.writebackSecret(channel), alongside VERSION_INCOMPATIBLE;
keep the undefined fallback only for expected absent-secret cases.
|
Working tree is clean — I made no edits (no mechanical fixes were needed). All verification was non-destructive. Review of PR #1215 —
|
Addresses bot review on #1215: - spawn(): listen for the child's async 'error' event so a missing binary / bad RELAYFILE_BIN maps to DAEMON_UNAVAILABLE instead of crashing the CLI. - rawRequest(): add a per-request timeout so a hung socket rejects with DAEMON_UNAVAILABLE instead of blocking startDaemonAndConnect forever. - listen for 'error' on the response stream. - compareSemver(): default partial-semver components to 0 (avoid NaN). - assertRelayfileVersion(): throw a typed RelayfileControlPlaneError ('VERSION_INCOMPATIBLE') so callers re-throwing daemon/version failures by code don't silently swallow it. - resolveWritebackBinding(): re-throw DAEMON_UNAVAILABLE / VERSION_INCOMPATIBLE rather than masking a daemon outage as a missing secret. - test: auto-start with a missing binary fails fast with DAEMON_UNAVAILABLE. - CHANGELOG: [Unreleased] entry for the control-plane socket migration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli/lib/relayfile-client.ts (1)
322-345: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPoll requests can overrun
startTimeoutMs.The default
requestTimeoutMs(10000) is larger than the defaultstartTimeoutMs(5000). During auto-start polling, eachhello()carries the full per-request timeout, so once the socket exists but the daemon wedges mid-handshake, a singlehello()can block ~10s and blow past the 5s start budget thatstartTimeoutMsdocuments ("how long to wait for an auto-started daemon to answer /v1/hello"). Before the socket exists this is harmless (fast ENOENT/ECONNREFUSED), but a slow-initializing daemon makes the budget non-binding.Consider bounding each poll by the remaining deadline.
🤖 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/lib/relayfile-client.ts` around lines 322 - 345, The auto-start polling in relayfile-client’s startup path can exceed the documented start budget because hello() uses the full request timeout on each retry. Update the polling loop in the startup/auto-start logic around hello() so each attempt is bounded by the remaining time until the deadline, rather than requestTimeoutMs, and keep the existing RelayfileControlPlaneError handling for spawnError and timeout failure.
🤖 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.
Outside diff comments:
In `@packages/cli/src/cli/lib/relayfile-client.ts`:
- Around line 322-345: The auto-start polling in relayfile-client’s startup path
can exceed the documented start budget because hello() uses the full request
timeout on each retry. Update the polling loop in the startup/auto-start logic
around hello() so each attempt is bounded by the remaining time until the
deadline, rather than requestTimeoutMs, and keep the existing
RelayfileControlPlaneError handling for spawnError and timeout failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af647b9f-c49a-4a33-b3f5-8ddcd5332107
📒 Files selected for processing (4)
CHANGELOG.mdpackages/cli/src/cli/commands/integration.tspackages/cli/src/cli/lib/relayfile-client.test.tspackages/cli/src/cli/lib/relayfile-client.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/cli/commands/integration.ts
relayfile already released 0.10.16 for the native-resource bind fix, so the control-plane + @relayfile/client release target moves to 0.10.17. Align the relay compat gate and version-gate tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review feedback addressed (pushed):
On the 448 tests green, tsc + eslint clean, codegen reproducible. |
…l-plane-client # Conflicts: # packages/cli/src/cli/commands/integration.ts
Picks up the /v1/hello X-Relayfile-API-Version header docs added on the relayfile side, regenerates the client types, and prettier-ignores the vendored spec so it stays byte-identical to the authoritative source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| it('rejects a version older than the minimum with an actionable message', () => { | ||
| expect(() => assertRelayfileVersion('0.10.15')).toThrow( | ||
| new RegExp( | ||
| `relayfile >= ${MIN_RELAYFILE_VERSION.replace(/\./g, '\\.')} is required; found "0\\.10\\.15"` |
@relayfile/client@0.10.17 is published, so swap the integration bridge onto the published package and delete the in-repo interim client + vendored OpenAPI + local codegen (openapi-typescript devDep, codegen:relayfile script). The client's lifecycle/version tests now live in the package; relay keeps the bridge-level real-daemon contract test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Final swap landed ✅ —
So the PR is no longer publish-gated — it's the end state: agent-relay drives integration ops through the published, version-negotiated |
Pick up the latest published @relayfile/client. API version (1) and the daemon min (0.10.17) are unchanged, so the wire contract and version gate are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
What
Replaces agent-relay's stringly-typed
spawn('relayfile', …)+ parse-stdout bridge with a typed client over relayfile's control-plane unix socket (relayfile control-plane serve). Integration ops (connect/bind/list/unbind/resolve/writeback) now go over a versioned, schema-checked wire instead of argv-as-request / stdout-as-response.This is the relay half of the control-plane RFC. The relayfile half (daemon + OpenAPI +
@relayfile/clientpackage +0.10.17) is a paired PR on the relayfile repo.Why
The CLI-as-RPC contract produced a recurring class of compile-time-preventable bugs that only surfaced at runtime (e.g.
flag provided but not defined: -json,pathGlobvsresource, native-resource passed as a VFS glob). The socket contract is version-negotiated via/v1/helloand the request/response types are generated from the OpenAPI, so field drift is a compile error, not a runtime surprise.Changes
relayfile-client.ts— typed control-plane client: HTTP/JSON overhttp.request({ socketPath })(zero deps),X-Relayfile-API-Versionhandshake, daemon auto-start (one lifecycle launch) +RELAYFILE_REQUIRE_DAEMON=1strict never-spawn mode, typed methods aliasing the generated schemas.defaultRelayfileBridgeswapped onto the client;runRelayfile/spawndeleted. TheRelayfileBridgeinterface is unchanged — subscribe/unsubscribe callers are untouched.--resourceto relayfile's stored path-glob viaresolve-pathbefore keying find/unbind (fixes the silent native-vs-glob mismatch).openapi/relayfile-control-plane-v1.openapi.yaml(npm run codegen:relayfile, reproducible).*.gen.tslint-ignored.Verification
RELAYFILE_BINis set, skip otherwise), tsc + eslint clean, codegen reproducible.relayfile control-plane servedaemon built from the paired relayfile branch.--version). The relayfile PR must publish first; the compat gate intentionally blocks older daemons.@relayfile/clientpackage and deleting the vendored spec/codegen (the package is built + green on the relayfile side; @worker owns its publish).RELAYFILE_BINso the contract tests run in CI (deferred until0.10.17is on npm).🤖 Generated with Claude Code