feat(cli): provision server-side inbound integration bridge in integration subscribe#1222
Conversation
…ration subscribe`
`agent-relay integration subscribe` now creates a relaycast inbound-target and a
relayfile-cloud webhook subscription (via the relayfile control-plane) so provider
messages are injected server-side into the target relay channel, with rollback on
partial failure and `{ok,data}` envelope parsing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds server-side inbound bridge provisioning to ChangesInbound bridge provisioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
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 |
![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 implements server-side inbound bridge wiring for the agent-relay integration subscribe command. It introduces the creation of a relaycast inbound target and a relayfile webhook subscription, enabling real provider messages to be injected directly into the target relay channel without requiring a local watcher process. Corresponding integration tests have been updated to mock and assert these new webhook subscription flows. There are no review comments, so I have no feedback to provide.
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.
|
✅ pr-reviewer applied fixes — committed and pushed Review: PR #1222 — Provision server-side inbound integration bridgeSummaryThis PR wires a server-side inbound bridge into I traced the changed exports — the new Verification (ran the way CI does, in this checkout)
Blocking finding (CI is red) — needs a human decision, left unchangedAll 9 new/updated tests in Root cause: the new The tests stub global Why I did not auto-fix it: the two candidate fixes are both human decisions per the review rules:
Recommended direction: since Advisory Notes
Addressed comments
The PR is not ready: a required check (the CLI vitest suite) is failing with 9 PR-introduced test failures, and resolving them requires a human decision (test-setup vs. production change) that I am not permitted to auto-apply. I have left the working tree unchanged (no edits made). |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| headers: { | ||
| Authorization: `Bearer ${workspaceKey}`, | ||
| 'Content-Type': 'application/json', | ||
| }, |
There was a problem hiding this comment.
Fixed in this PR: inbound-target provisioning now derives its destination only from explicit --base-url/env/default https://cast.agentrelay.com, enforces HTTPS, and is decoupled from the local-broker auth fallback; CodeQL Analysis passes on the new commits.
|
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. |
![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: 5f6e7156f2
ℹ️ 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".
| relayfileWebhook = await deps.relayfile.createWebhookSubscription({ | ||
| url: inboundTarget.url, | ||
| pathGlobs: [pathGlob], | ||
| secret: inboundTarget.secret, | ||
| }); |
There was a problem hiding this comment.
When integration subscribe succeeds, this creates a persistent relayfile-cloud webhook subscription, but the returned relayfileWebhook.subscriptionId is only used for rollback/logging; relayfile.bind still stores only the Relaycast writeback subscription.id. In the normal unsubscribe or same-resource re-subscribe path, runUnsubscribe/replacement cleanup has no way to call deleteWebhookSubscription, so provider events can keep forwarding to the inbound target after the CLI reports the binding removed, or duplicate after replacements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a valid follow-up, but it is intentionally out of scope for this PR: unsubscribe teardown of the relayfile-cloud webhook subscription is tracked separately. This PR keeps the existing relayfile binding contract storing the Relaycast subscription ID, and it preserves rollback for partial failures by deleting the newly-created relayfile webhook subscription if provisioning fails before the bind is committed. Persisting a second cloud subscription ID for normal unsubscribe/replacement teardown needs a binding-schema/API change and will be handled in that follow-up rather than expanding this bridge PR.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const inboundTarget = await createRelayfileInboundTarget(opts, local, { | ||
| channel, | ||
| provider, | ||
| pathGlob, | ||
| }); |
There was a problem hiding this comment.
🟡 Server-side inbound target is never cleaned up when the subscribe operation fails partway through
The server-side inbound target is provisioned (fetch(…POST…) at packages/cli/src/cli/commands/integration.ts:725-729) before the try/catch block that creates the webhook, relayfile subscription, and binding, so if any of those later steps fail, the catch block never rolls it back.
Impact: Each failed subscribe attempt leaks an orphaned inbound-target resource on the Relaycast server.
Inbound target created outside the rollback scope
The try/catch block at packages/cli/src/cli/commands/integration.ts:742-784 carefully rolls back three resources on failure: webhook (line 772-775), subscription (line 767-770), and relayfileWebhook (line 777-783). However, createRelayfileInboundTarget at line 725 is a server-side POST mutation that provisions a resource on the Relaycast API (/v1/integrations/relayfile/inbound-target). Because it is called before the try block, its result is never cleaned up if the try block throws.
Every other mutating step in the subscribe flow has explicit rollback. The inbound target should either be created inside the try block with corresponding rollback in the catch block, or the catch block should be extended to clean up the inbound target.
Prompt for agents
In runSubscribe (packages/cli/src/cli/commands/integration.ts), the createRelayfileInboundTarget call at line 725 provisions a server-side resource via POST but is placed outside the try/catch block that handles rollback (lines 742-784). If any step inside the try block fails, the inbound target is never cleaned up.
To fix this, either:
1. Move the createRelayfileInboundTarget call inside the try block and add a corresponding cleanup in the catch block (similar to how relayfileWebhook is cleaned up), or
2. Add the inbound target to the catch block's rollback logic. You would need to track the inbound target result and add a deletion API call (or a new deleteRelayfileInboundTarget helper) in the catch block.
Approach 1 is more consistent with the existing pattern where all mutations happen inside the try block.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is a valid lifecycle concern, but the current Relaycast inbound-target response/contract does not expose a target id or delete endpoint that the CLI can use for rollback. This PR preserves rollback for resources it owns handles for (Relaycast webhook/subscription and relayfile webhook subscription on pre-bind failure). Cleaning up server-side inbound-target allocations needs a control-plane delete/id contract change, so I am leaving it as a follow-up rather than inventing an uncallable rollback path in this CLI 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
🧹 Nitpick comments (1)
packages/cli/src/cli/commands/integration-subscribe.test.ts (1)
101-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared fetch stub into a test helper.
The fetch stub returning the
{ ok: true, data: { url, secret } }envelope is duplicated verbatim inrelaycast-groups.test.ts(lines 11-30). Extracting it into a shared helper (e.g.,stubInboundTargetFetch()) would reduce duplication and ensure both files stay in sync if the response shape changes.♻️ Optional refactor
+// e.g., in a shared test helper file +export function stubInboundTargetFetch() { + vi.stubGlobal( + 'fetch', + vi.fn( + async () => + new Response( + JSON.stringify({ + ok: true, + data: { + url: 'https://cast.test/v1/integrations/relayfile/inbound/ws/ch', + secret: 'inbound-secret', + }, + }), + { status: 201, headers: { 'content-type': 'application/json' } } + ) + ) + ); +}Then in both test files:
- vi.stubGlobal( - 'fetch', - vi.fn( - async () => - new Response( - JSON.stringify({ - ok: true, - data: { - url: 'https://cast.test/v1/integrations/relayfile/inbound/ws/ch', - secret: 'inbound-secret', - }, - }), - { status: 201, headers: { 'content-type': 'application/json' } } - ) - ) - ); + stubInboundTargetFetch();🤖 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 101 - 124, The fetch stub in this test is duplicated in another spec, so extract the shared `{ ok: true, data: { url, secret } }` response setup into a reusable test helper. Create a helper such as `stubInboundTargetFetch()` and use it both here and in `relaycast-groups.test.ts`, keeping the existing `vi.stubGlobal('fetch', ...)` behavior but centralizing the response envelope so both tests stay aligned if the shape changes.
🤖 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 449-452: The inbound target parsing in the relaycast response
handling only validates types, so blank trimmed values and non-HTTPS URLs can
still slip through. Update the validation in the relayfile inbound target path
(the block that returns the trimmed `url` and `secret`) to reject
whitespace-only `secret` and `url` values after trimming, and ensure the parsed
`url` is HTTPS before returning it. Keep the checks close to the existing
`isRecord` / `data.url` / `data.secret` guards so the function fails fast with
the same invalid-response error.
---
Nitpick comments:
In `@packages/cli/src/cli/commands/integration-subscribe.test.ts`:
- Around line 101-124: The fetch stub in this test is duplicated in another
spec, so extract the shared `{ ok: true, data: { url, secret } }` response setup
into a reusable test helper. Create a helper such as `stubInboundTargetFetch()`
and use it both here and in `relaycast-groups.test.ts`, keeping the existing
`vi.stubGlobal('fetch', ...)` behavior but centralizing the response envelope so
both tests stay aligned if the shape changes.
🪄 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: 328c42bf-35b6-4119-b0ca-0e6f5b4fe22a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
CHANGELOG.mdpackages/cli/src/cli/commands/core.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/entrypoint.test.ts
|
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
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="packages/cli/src/cli/commands/integration-subscribe.test.ts">
<violation number="1" location="packages/cli/src/cli/commands/integration-subscribe.test.ts:134">
P3: Defaulting the harness to a workspaceKey makes the suite always run the workspace-aware path. That can hide regressions in the command’s fallback when no local relay options are available.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| relayfile: relayfile as never, | ||
| resolveLocalRelayOptions: async () => undefined, | ||
| resolveLocalRelayOptions: | ||
| opts.resolveLocalRelayOptions ?? (async () => ({ workspaceKey: 'rk_live_test' })), |
There was a problem hiding this comment.
P3: Defaulting the harness to a workspaceKey makes the suite always run the workspace-aware path. That can hide regressions in the command’s fallback when no local relay options are available.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/commands/integration-subscribe.test.ts, line 134:
<comment>Defaulting the harness to a workspaceKey makes the suite always run the workspace-aware path. That can hide regressions in the command’s fallback when no local relay options are available.</comment>
<file context>
@@ -104,7 +130,8 @@ function harness(
relayfile: relayfile as never,
- resolveLocalRelayOptions: async () => undefined,
+ resolveLocalRelayOptions:
+ opts.resolveLocalRelayOptions ?? (async () => ({ workspaceKey: 'rk_live_test' })),
isInteractive: () => false,
log,
</file context>
| opts.resolveLocalRelayOptions ?? (async () => ({ workspaceKey: 'rk_live_test' })), | |
| opts.resolveLocalRelayOptions ?? (async () => undefined), |
There was a problem hiding this comment.
Fixed in d0f6fb9 with a regression test that bypasses the harness default workspace key: no --workspace-key, blank RELAY_WORKSPACE_KEY/RELAY_API_KEY, isolated empty AGENT_RELAY_HOME, and resolveLocalRelayOptions returns undefined. The command now has explicit coverage for the fallback/no-workspace path and fails before any fetch or provisioning side effects.
There was a problem hiding this comment.
Nice — the no-workspace fallback is now covered explicitly, and the parent concern is addressed for this PR.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
|
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. |
There was a problem hiding this comment.
1 issue found across 1 file (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="packages/cli/src/cli/commands/integration-subscribe.test.ts">
<violation number="1" location="packages/cli/src/cli/commands/integration-subscribe.test.ts:134">
P3: Defaulting the harness to a workspaceKey makes the suite always run the workspace-aware path. That can hide regressions in the command’s fallback when no local relay options are available.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
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. |
|
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. |
2 participants
Part of the inbound integration bridge (provider → relay agent, server-side, no client process). Companion PRs: relaycast (ingress), relaycast-cloud (secret+expose), relay (CLI subscribe), relayfile (control-plane proxy), relayfile-cloud (snapshot enrichment). Cross-repo — review/merge together.
agent-relay integration subscribenow creates a relaycast inbound-target + a relayfile-cloud webhook subscription (via the relayfile control-plane), with rollback on partial failure and{ok,data}envelope parsing. CHANGELOG updated.Tests: 29 passing (integration-subscribe, relaycast-groups) +
tsc.Review status
3-way fresh-eyes review completed. Fixed: fail-closed HMAC secret normalization, 5xx-on-transient (no silent loss), require-dedupe-key (no double-inject). Verified correct: exactly-once node delivery, snapshot bounds, signing order.
unsubscribemust delete the relayfile-cloud webhook_subscription (persist its id on the binding) — currently unsubscribe does not stop inbound delivery (cross-repo: relay CLI + relayfile daemon)cast.agentrelay.comtoRELAYFILE_WEBHOOK_HOST_ALLOWLISTfor the preview/prod host🤖 Generated with Claude Code