feat: consume durable Relayfile babysitter subscriptions#100
feat: consume durable Relayfile babysitter subscriptions#100miyaontherelay wants to merge 3 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 34 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 selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds a Relayfile resource-subscriptions client and exposes it through mount clients. FactoryLoop now persists babysitter subscription state, routes durable delivery claims, retries acceptance, handles terminal deliveries, and falls back to local routing when the service is unavailable. ChangesDurable resource subscription delivery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant FactoryLoop
participant Babysitter
Provider->>FactoryLoop: deliver PR event
FactoryLoop->>Provider: claim delivery
Provider-->>FactoryLoop: delivery claim
FactoryLoop->>Babysitter: queue wake
FactoryLoop->>Provider: accept delivery claim
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
khaliqgant
left a comment
khaliqgant
left a comment
There was a problem hiding this comment.
Aligned on the hard part (durable ingress), with one convergence note on the last mile. Reviewing for architectural fit with the escalation redesign (#99), not line-by-line.
Aligned — this is the right substrate
Durable resource subscriptions with Relayfile owning matching + delivery-claim persistence (deliveryId/claimToken, terminal quarantine, restart-survivable hand-off) is exactly the primitive the system needs. It fixes cloud#2670's "subscriptions terminate at the orchestrator" — the orchestrator now durably receives events for a resource it owns instead of losing them on restart.
Importantly, #99 should ride on this, not reinvent it. The "watch the issue for the human's answer" half of the escalation redesign is the same shape as this PR's "watch the PR for babysitter events" — a durable subscription to a resource's changes. So this is a complementary enabler, and consolidating both onto one durable-subscription layer is the goal.
Convergence note — the wake's last mile still injects into a live agent
The event now arrives durably, but it's still delivered into the babysitter by injection: #waitForInjectedAndSubmit(critical.input) for the wake, #fleet.sendInput(recipient, githubReplyEvent(...)), and the #injectPendingSlack/GithubClarification paths. That's the fragile primitive #99 argues should become release/resume + spawn-task delivery.
Not a conflict — a layering distinction:
- This PR = durable event ingress (orchestrator reliably gets the event). ✅
- #99 = durable agent delivery (agent receives it without live injection). ⟵ the last inch this PR still does the old way.
The babysitter is often a claude agent, which is less injection-racy than the codex implementer that exposed this (#98) — so it'll hit the wall less often, but it's the same wall (PTY injection into a live TUI agent; delivery_injected races the agent's state). A durable subscription undone by a fragile last-inch injection is a shame.
Suggestion: land the durable-subscription layer as-is (it's foundational and correct), and track "move wake delivery off waitForInjected/sendInput onto #99's release/resume + spawn-task model" as the follow-up so ingress and delivery converge. That also lets #99's ask/answer path reuse this exact subscription machinery for the issue-comment channel.
(Noting the PR's own gate: "do not merge until the canonical AgentWorkforce/relayfile pin lands" — so this is blocked on that regardless.)
Refs: #99 (escalation redesign — this is the ingress substrate it should reuse), #98 (the injection race that motivates moving delivery off injection), #87 (babysitter wake), cloud#2670 (subscriptions terminate at orchestrator — fixed here).
|
Agreed. This PR intentionally owns durable resource matching/claim persistence plus the crash-safe hand-off into Factory’s existing babysitter wake path; it does not redesign the final agent-delivery mechanism. I will track the #99 release/resume + spawn-task model as the follow-up convergence point for both babysitter wakes and human-answer escalation delivery. The documented relayfile OpenAPI/SDK and lead gates remain in place, so this PR stays unmerged. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/orchestrator/factory.ts (1)
5252-5268: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCancel before deleting the subscription reference.
In
#completeIssue, Line 6554 deletes#babysitterPrbefore calling this method. Consequently,refis undefined on local completion and the subscription DELETE is never sent. Let#cancelBabysitterWakeown map deletion or pass the captured reference explicitly.🤖 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 `@src/orchestrator/factory.ts` around lines 5252 - 5268, The `#completeIssue` flow removes the `#babysitterPr` entry before `#cancelBabysitterWake` can cancel its durable subscription, preventing the DELETE on local completion. Update `#cancelBabysitterWake` and its caller so cancellation receives the captured subscription reference or performs `#babysitterPr` deletion itself, ensuring resourceSubscriptions.cancel runs before the reference is discarded.
🧹 Nitpick comments (1)
src/orchestrator/factory.test.ts (1)
12074-12085: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel idempotent acceptance and test post-accept crash recovery.
The fake rejects a repeated acceptance after deleting the lease, while restoration relies on retrying a claim that Relayfile may already have accepted. Cache accepted receipts by delivery/token and add a restart test where remote acceptance succeeds but local persistence initially fails.
🤖 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 `@src/orchestrator/factory.test.ts` around lines 12074 - 12085, Update the fake’s acceptDelivery method to cache successful acceptance receipts keyed by delivery ID and claim token, returning the cached receipt for repeated valid requests instead of rejecting after lease deletion. Add a restart/crash-recovery test covering remote acceptance succeeding while local persistence initially fails, then verify restoration retries acceptance successfully and completes using the cached receipt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/orchestrator/factory.ts`:
- Around line 230-246: Update the babysitter subscription maintenance flow to
renew active nonterminal subscriptions before
BABYSITTER_SUBSCRIPTION_TTL_SECONDS elapses, rather than relying only on PR
metadata events. Extend the retry drive to invoke the subscription createOrRenew
operation, so transient initial creation or renewal failures are retried
alongside claim/accept operations. Preserve the existing terminal subscription
handling and event-type distinctions.
- Around line 5980-5984: Update the closed, unmerged handling in the surrounding
babysitter event flow so terminal claims from a real pull_request.closed
metadata event are routed before cancellation and early return. Ensure the final
durable wake/quarantine path runs before closed-state teardown, while preserving
the existing teardown behavior after routing.
In `@src/subscriptions/resource-subscriptions.ts`:
- Around line 84-109: Update createResourceSubscriptionsHttpClient and its
ResourceSubscriptionsHttpClientOptions to support a configurable request timeout
and abort signal for every fetch. Combine the timeout with any caller-provided
init.signal, abort stalled requests, and translate timeout aborts into
ResourceSubscriptionsHttpError while preserving normal abort and response
handling.
- Around line 135-141: Update the delivery extraction logic around
parseDeliveryClaim so non-array payload objects must contain a valid deliveries
array; otherwise reject the malformed envelope instead of returning an empty
list. Preserve direct array payload handling and ensure the rejection propagates
so durable polling cannot be treated as a successful no-claims result.
---
Outside diff comments:
In `@src/orchestrator/factory.ts`:
- Around line 5252-5268: The `#completeIssue` flow removes the `#babysitterPr` entry
before `#cancelBabysitterWake` can cancel its durable subscription, preventing the
DELETE on local completion. Update `#cancelBabysitterWake` and its caller so
cancellation receives the captured subscription reference or performs
`#babysitterPr` deletion itself, ensuring resourceSubscriptions.cancel runs before
the reference is discarded.
---
Nitpick comments:
In `@src/orchestrator/factory.test.ts`:
- Around line 12074-12085: Update the fake’s acceptDelivery method to cache
successful acceptance receipts keyed by delivery ID and claim token, returning
the cached receipt for repeated valid requests instead of rejecting after lease
deletion. Add a restart/crash-recovery test covering remote acceptance
succeeding while local persistence initially fails, then verify restoration
retries acceptance successfully and completes using the cached receipt.
🪄 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: 8608b0c0-a7a5-4d40-92d5-9fa7ccf1317c
📒 Files selected for processing (12)
src/index.tssrc/mount/relayfile-cloud-mount-client.test.tssrc/mount/relayfile-cloud-mount-client.tssrc/orchestrator/factory.test.tssrc/orchestrator/factory.tssrc/ports/mount.tssrc/ports/state.tssrc/state/file-state-store.test.tssrc/state/file-state-store.tssrc/subscriptions/index.tssrc/subscriptions/resource-subscriptions.tssrc/testing/fakes.ts
|
Follow-up on CodeRabbit’s outside-diff and test-double notes: |
2 participants
Summary
Temporary upstream contract pin
Validated against relayfile-cloud #126 at
ba2aed227982523907e9b367beed0e3f212b6e2d.Do not merge, release, or deploy until the canonical AgentWorkforce/relayfile OpenAPI/SDK pin is available and the lead approves this exact head.
Verification
npm run buildnpx vitest run src/orchestrator/factory.test.ts --reporter=dot(316 passed)npx vitest run src/mount/relayfile-cloud-mount-client.test.ts src/state/file-state-store.test.ts --reporter=dot(49 passed)