feat: add relayfile control-plane client#344
Conversation
|
Warning Review limit reached
Next review available in: 55 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 (4)
📒 Files selected for processing (29)
✨ 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 a versioned local control-plane API for the relayfile daemon/CLI, implementing a Unix domain socket server with endpoints for managing integration bindings, resolving resource paths, and checking provider status. It also adds a typed TypeScript client (@relayfile/client) generated from the OpenAPI contract. The review feedback identifies several important issues: potential race conditions during concurrent binding updates, a security window where the Unix socket is briefly accessible to other local users before permissions are restricted, missing timeouts on both the Go HTTP server and the TypeScript client, incorrect response header ordering in error handling, potential NaN errors in semver comparison, and a directory traversal vulnerability in workspace path resolution.
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.
| func handleControlPlaneBind(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| writeControlPlaneError(w, http.StatusMethodNotAllowed, controlPlaneErrInvalidArgument, "method not allowed") | ||
| return | ||
| } | ||
| var req bindRequest | ||
| if err := decodeControlPlaneJSON(r, &req); err != nil { | ||
| writeControlPlaneError(w, http.StatusBadRequest, controlPlaneErrInvalidArgument, err.Error()) | ||
| return | ||
| } | ||
| binding, replaced, warning, err := bindRelayIntegration(relayIntegrationBindInput{ | ||
| Provider: req.Provider, | ||
| Resource: req.Resource, | ||
| Channel: req.Channel, | ||
| WebhookID: req.WebhookID, | ||
| WebhookToken: req.WebhookToken, | ||
| SubscriptionID: req.SubscriptionID, | ||
| }) | ||
| if err != nil { | ||
| writeControlPlaneMappedError(w, err) | ||
| return | ||
| } | ||
| writeControlPlaneJSON(w, http.StatusOK, bindResponse{Binding: binding, Replaced: replaced, Warning: warning}) | ||
| } |
There was a problem hiding this comment.
Since the control plane HTTP server handles requests concurrently, multiple concurrent requests to /v1/integrations/bind or /v1/integrations/unbind will read and write the same bindings JSON file concurrently. This can lead to race conditions, lost updates, or file corruption. To prevent this, introduce a package-level sync.Mutex in control_plane.go and use it to synchronize access to bindRelayIntegration, unbindRelayIntegration, and readRelayIntegrationBindings in their respective HTTP handlers.
| if err := os.MkdirAll(filepath.Dir(sock), 0o700); err != nil { | ||
| return err | ||
| } | ||
| _ = os.Remove(sock) | ||
| listener, err := net.Listen("unix", sock) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer listener.Close() | ||
| defer os.Remove(sock) | ||
| if err := os.Chmod(sock, 0o600); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When the Unix socket is created in a shared directory like /tmp (which is the default when XDG_RUNTIME_DIR and RELAYFILE_SOCK are unset), net.Listen creates the socket file with permissions governed by the process's umask. There is a brief window of time between net.Listen and os.Chmod where other local users could potentially connect to the socket if the umask is permissive. To prevent this, consider setting a restrictive umask (e.g., 0077) before listening, or creating a dedicated subdirectory with 0700 permissions to house the socket file.
| return err | ||
| } | ||
|
|
||
| server := &http.Server{Handler: newControlPlaneHandler()} |
There was a problem hiding this comment.
The http.Server is created without any timeouts configured. To prevent resource exhaustion and potential denial-of-service (DoS) from slow or hanging connections, it is highly recommended to set explicit timeouts (ReadTimeout, WriteTimeout, and IdleTimeout).
server := &http.Server{
Handler: newControlPlaneHandler(),
ReadTimeout: 5 * time.Second,
WriteTimeout: 5 * time.Second,
IdleTimeout: 15 * time.Second,
}| func writeControlPlaneJSON(w http.ResponseWriter, status int, value any) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(status) | ||
| payload, err := json.MarshalIndent(value, "", " ") | ||
| if err != nil { | ||
| _, _ = w.Write([]byte(`{"error":{"code":"DAEMON_UNAVAILABLE","message":"failed to encode response"}}` + "\n")) | ||
| return | ||
| } | ||
| payload = append(payload, '\n') | ||
| _, _ = w.Write(payload) | ||
| } |
There was a problem hiding this comment.
In writeControlPlaneJSON, w.WriteHeader(status) is called before json.MarshalIndent. If the JSON marshalling fails, the header has already been sent, meaning the client will receive a misleading 200 OK (or other success status) followed by the error JSON body. Marshalling the payload first allows you to correctly return a 500 Internal Server Error if encoding fails.
func writeControlPlaneJSON(w http.ResponseWriter, status int, value any) {
payload, err := json.MarshalIndent(value, "", " ")
if err != nil {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusInternalServerError)
_, _ = w.Write([]byte(`{"error":{"code":"DAEMON_UNAVAILABLE","message":"failed to encode response"}}` + "\n"))
return
}
payload = append(payload, '\n')
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(status)
_, _ = w.Write(payload)
}| export function compareSemver(a: string, b: string): number { | ||
| const core = (v: string) => v.split(/[-+]/)[0]!.split('.').map((n) => Number.parseInt(n, 10) || 0); | ||
| 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.
If compareSemver is called with a 2-part version or a non-standard version string, core(v) might return an array with fewer than 3 elements, resulting in undefined values and causing the subtraction to return NaN. Since this function is exported, it should be made robust against such inputs by defaulting missing parts to 0.
| export function compareSemver(a: string, b: string): number { | |
| const core = (v: string) => v.split(/[-+]/)[0]!.split('.').map((n) => Number.parseInt(n, 10) || 0); | |
| const [a0, a1, a2] = core(a); | |
| const [b0, b1, b2] = core(b); | |
| return (a0! - b0!) || (a1! - b1!) || (a2! - b2!); | |
| } | |
| export function compareSemver(a: string, b: string): number { | |
| const core = (v: string) => v.split(/[-+]/)[0]!.split('.').map((n) => Number.parseInt(n, 10) || 0); | |
| const aParts = core(a); | |
| const bParts = core(b); | |
| const a0 = aParts[0] ?? 0; | |
| const a1 = aParts[1] ?? 0; | |
| const a2 = aParts[2] ?? 0; | |
| const b0 = bParts[0] ?? 0; | |
| const b1 = bParts[1] ?? 0; | |
| const b2 = bParts[2] ?? 0; | |
| return (a0 - b0) || (a1 - b1) || (a2 - b2); | |
| } |
| const req = request( | ||
| { | ||
| socketPath: this.socketPath, | ||
| method: opts.method, | ||
| path, | ||
| headers: { | ||
| 'X-Relayfile-API-Version': String(RELAYFILE_API_VERSION), | ||
| Accept: 'application/json', | ||
| ...(payload | ||
| ? { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(payload) } | ||
| : {}), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The HTTP request to the local Unix socket does not have any timeout configured. If the daemon hangs or becomes unresponsive, the client request will hang indefinitely. It is recommended to set a default timeout (e.g., 15 seconds) and handle the 'timeout' event to destroy the request and reject the promise.
const req = request(
{
socketPath: this.socketPath,
method: opts.method,
path,
timeout: 15000,
headers: {
'X-Relayfile-API-Version': String(RELAYFILE_API_VERSION),
Accept: 'application/json',
...(payload
? { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(payload) }
: {}),
},
},| func readMountedJSON(remotePath string, out any) bool { | ||
| record, ok := activeWorkspaceRecordForMountResolution() | ||
| if !ok || strings.TrimSpace(record.LocalDir) == "" { | ||
| return false | ||
| } | ||
| localPath := filepath.Join(record.LocalDir, filepath.FromSlash(strings.TrimPrefix(remotePath, "/"))) | ||
| payload, err := os.ReadFile(localPath) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return json.Unmarshal(payload, out) == nil | ||
| } |
There was a problem hiding this comment.
In readMountedJSON, localPath is constructed by joining record.LocalDir with remotePath. If remotePath contains directory traversal sequences (like ..), it could potentially escape record.LocalDir. Although current callers slugify or sanitize inputs, it is highly recommended to implement defensive path traversal checks to ensure localPath remains strictly within record.LocalDir.
func readMountedJSON(remotePath string, out any) bool {
record, ok := activeWorkspaceRecordForMountResolution()
if !ok || strings.TrimSpace(record.LocalDir) == "" {
return false
}
localPath := filepath.Join(record.LocalDir, filepath.FromSlash(strings.TrimPrefix(remotePath, "/")))
rel, err := filepath.Rel(record.LocalDir, localPath)
if err != nil || strings.HasPrefix(rel, "..") {
return false
}
payload, err := os.ReadFile(localPath)
if err != nil {
return false
}
return json.Unmarshal(payload, out) == nil
}![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: f9c60d083c
ℹ️ 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 autoStart is left at its default and relayfile/RELAYFILE_BIN is missing or not executable, Node's spawn() returns a child and emits the failure asynchronously, so this try/catch does not catch it. Because no error listener is attached, the consumer process gets an uncaught exception instead of a rejected DAEMON_UNAVAILABLE error. Attach an error handler before polling (or otherwise convert the spawn failure into the intended control-plane error).
Useful? React with 👍 / 👎.
| type relayIntegrationUnbindResult struct { | ||
| Provider string | ||
| PathGlob string | ||
| Removed int | ||
| Warning string |
There was a problem hiding this comment.
When /v1/integrations/unbind returns this value, encoding/json uses these exported Go field names because the new response struct has no JSON tags, so the daemon emits Provider, PathGlob, and Removed instead of the OpenAPI-documented provider, pathGlob, and removed. OpenAPI/JS clients reading the documented fields will see them as missing even though the operation succeeded; add lower-camel JSON tags or a tagged response DTO.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| type relayIntegrationUnbindResult struct { | ||
| Provider string | ||
| PathGlob string | ||
| Removed int | ||
| Warning string | ||
| } |
There was a problem hiding this comment.
🔴 Unbind response fields are invisible to all non-Go API clients
The unbind result struct is serialized without JSON field-name tags (relayIntegrationUnbindResult at cmd/relayfile-cli/main.go:2480-2485), so all four fields arrive as PascalCase instead of the camelCase the OpenAPI contract promises.
Impact: Every TypeScript (or other non-Go) consumer of the /v1/integrations/unbind endpoint receives undefined for provider, pathGlob, removed, and warning.
Go struct lacks json tags while every other response type has them
The struct at cmd/relayfile-cli/main.go:2480-2485:
type relayIntegrationUnbindResult struct {
Provider string
PathGlob string
Removed int
Warning string
}Go's json.Marshal produces {"Provider":"slack","PathGlob":"/slack/channels/**","Removed":1,"Warning":""} — PascalCase.
The OpenAPI spec at openapi/relayfile-control-plane-v1.openapi.yaml:457-468 declares the fields as provider, pathGlob, removed, warning (camelCase), and the generated TypeScript type at packages/client/src/generated/control-plane.ts:257-262 expects the same.
Every other response struct serialized by the control-plane has proper JSON tags (e.g., bindResponse at cmd/relayfile-cli/control_plane.go:98-102, helloResponse at line 45-49). This struct is the only one missing them.
The Go test at cmd/relayfile-cli/control_plane_test.go:113-122 deserializes into the same Go struct, so PascalCase round-trips fine and the test passes — but any external client using the OpenAPI-generated types sees all fields as undefined.
| type relayIntegrationUnbindResult struct { | |
| Provider string | |
| PathGlob string | |
| Removed int | |
| Warning string | |
| } | |
| type relayIntegrationUnbindResult struct { | |
| Provider string `json:"provider"` | |
| PathGlob string `json:"pathGlob"` | |
| Removed int `json:"removed"` | |
| Warning string `json:"warning,omitempty"` | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| case strings.Contains(message, "PATH_GLOB") || strings.Contains(message, "resource"): | ||
| status = http.StatusUnprocessableEntity | ||
| code = controlPlaneErrResourceUnresolved |
There was a problem hiding this comment.
🟡 Errors from connect and writeback endpoints can be misclassified as resource-resolution failures
Any error whose message contains the common word "resource" is classified as a 422 resource-resolution failure (strings.Contains(message, "resource") at cmd/relayfile-cli/control_plane.go:527), even when the error originates from unrelated operations like provider connect or writeback-secret.
Impact: Clients may receive a misleading 422 "RESOURCE_UNRESOLVED" status instead of the correct error code when a connect or writeback call fails with a message that happens to include "resource".
writeControlPlaneMappedError is shared across all handlers but the "resource" match is too broad
writeControlPlaneMappedError at cmd/relayfile-cli/control_plane.go:519-534 is called from seven different handlers (lines 281, 329, 352, 383, 396, 414, 436). The strings.Contains(message, "resource") check at line 527 was designed to catch errors from resolveIntegrationBindPathGlob (e.g., "PATH_GLOB/resource is required", "%s resource is empty after trimming sigils"), but it also fires for errors from handleControlPlaneConnectProvider (line 329) and handleControlPlaneWritebackSecret (line 436).
For example, runIntegrationConnect can call listAccessibleResources at cmd/relayfile-cli/main.go:2764 which returns "list %s accessible resources: %w" — this contains "resource" and would be misclassified as 422/RESOURCE_UNRESOLVED instead of a connection error.
A narrower match (e.g., checking for specific error prefixes or using structured error types) would prevent the false positive.
Prompt for agents
The writeControlPlaneMappedError function at cmd/relayfile-cli/control_plane.go:519-534 uses strings.Contains(message, "resource") to classify errors as RESOURCE_UNRESOLVED (422). This is too broad because the function is called from all control-plane handlers, not just the resolve-path handler. Errors from runIntegrationConnect (e.g. "list jira accessible resources: ...") or other handlers that happen to contain the word "resource" get misclassified.
Possible fixes:
1. Use more specific string patterns (e.g. check for known error prefixes from resolveIntegrationBindPathGlob).
2. Use structured error types (e.g. a custom error interface with a Code() method) so the mapper can inspect the error type rather than string-matching.
3. Move the resource-specific error mapping closer to the handlers that deal with resource resolution (resolve-path, bind, unbind) instead of applying it globally.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func activeWorkspaceRecordForMountResolution() (workspaceRecord, bool) { | ||
| activeWorkspaceRecordCacheOnce.Do(func() { | ||
| creds, _ := loadCredentials() | ||
| name, _ := activeWorkspaceName(resolveToken("", creds)) | ||
| if strings.TrimSpace(name) == "" { | ||
| return | ||
| } | ||
| if record, ok := workspaceRecordByName(name); ok { | ||
| activeWorkspaceRecordCache = record | ||
| activeWorkspaceRecordCacheOK = true | ||
| return | ||
| } | ||
| if record, ok := workspaceRecordByID(name); ok { | ||
| activeWorkspaceRecordCache = record | ||
| activeWorkspaceRecordCacheOK = true | ||
| return | ||
| } | ||
| }) | ||
| return activeWorkspaceRecordCache, activeWorkspaceRecordCacheOK |
There was a problem hiding this comment.
🚩 Process-level sync.Once cache for workspace resolution may go stale in long-lived daemon
The activeWorkspaceRecordForMountResolution() function at cmd/relayfile-cli/main.go:8341-8359 uses a sync.Once to cache the active workspace record for the lifetime of the process. This is fine for short-lived CLI invocations, but the control-plane server (control-plane serve) is a long-lived daemon. If the user switches workspaces while the daemon is running (e.g., relayfile workspace use other-ws), the daemon will continue resolving native resources against the old workspace's mount directory. The resetActiveWorkspaceRecordForMountResolutionCache() function exists but is only called from test helpers (cmd/relayfile-cli/main_test.go:2803), not from any production code path. This may be acceptable if daemon restarts are expected on workspace changes, but it's worth confirming that expectation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| args = append(args, "--wait-sync") | ||
| } | ||
| var out bytes.Buffer | ||
| if err := runIntegrationConnect(args, strings.NewReader(""), &out); err != nil { |
There was a problem hiding this comment.
🚩 Connect endpoint passes empty stdin, blocking interactive Atlassian flows
The control-plane connect handler at cmd/relayfile-cli/control_plane.go:328 calls runIntegrationConnect(args, strings.NewReader(""), &out) with an empty reader as stdin. For Atlassian providers (Jira, Confluence), runIntegrationConnect calls runAtlassianSitePicker at cmd/relayfile-cli/main.go:2701-2704 which may prompt for interactive site selection via stdin. With an empty reader, the prompt would read an empty string and likely fail or select a default. This is probably acceptable since the control-plane is designed for non-interactive use and callers should use the noOpen: true option, but Atlassian connect via the control-plane socket will likely fail or behave unexpectedly without documenting this limitation.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
gofmt reports no formatting issues. Everything is clean and verified. Let me write the final review. Review: PR #344 —
|
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
Review: PR #344 — Integration native resource globs / control-plane clientSummaryThis PR adds a versioned local control-plane to the relayfile CLI ( I traced the diff across callers, types, generated artifacts, the OpenAPI spec, lockfiles, and CI workflow, and verified it the way CI does. Verification (ran the canonical build/test end-to-end)Go (matched
Client package (exactly the new
Cross-checked that every type/function the new No source edits were required — the working tree is clean (only gitignored Addressed comments
Observations (non-blocking, no change made)
Advisory Notes
This PR is mechanically and contractually sound on the current checkout. However, I cannot confirm the live CI run status, merge-conflict/mergeability state, or that all required checks have completed and passed — those are reported by the harness post-run, not something I can observe here. Because I cannot verify that every required check has completed and is green, I am not printing READY. |
1 participant
Summary
relayfile control-plane servewith a versioned local HTTP/JSON API over a Unix socket, including/v1/helloAPI negotiationopenapi/relayfile-control-plane-v1.openapi.yamlcontract and generated@relayfile/clientpackage0.10.16, including parseable Go-binaryrelayfile --versionoutput andbind --list --jsoncompatibility@relayfile/clientinto root workspaces, CI codegen drift checks, changelogs, and the publish workflowRelease / Merge Ordering
relayfile/@relayfile/client0.10.16before feat(cli): drive relayfile integration ops over the control-plane socket (no shell-out) relay#1215 can complete its final dependency swap.Verification
npm run codegen --workspace=@relayfile/client && git diff --exit-code -- packages/client/src/generated/control-plane.tsnpm run typecheck --workspace=@relayfile/clientnpm run build --workspace=@relayfile/clientnpm run test --workspace=@relayfile/clientRELAYFILE_BIN=/tmp/relayfile-client-check/relayfile npm run test --workspace=@relayfile/clientnpm pack --workspace=@relayfile/client --dry-runnpm ci --dry-runnpm run buildnpm run typechecknpm run testgo test ./...scripts/check-contract-surface.shgo run ./cmd/relayfile-cli --version->0.10.16HOME=$(mktemp -d) go run ./cmd/relayfile-cli integration bind --list --json->[]