feat(server): device workers can claim runs in orgs they're pinned to#1149
Conversation
A user-scoped device worker's claim scope was [token's bound org, personal org], so a watcher/connection pinned to the device in another org the owner belongs to could never be claimed (the run sat pending — the server skips device-pinned runs). The pin is already the owner's consent (evaluateDeviceWorkerAccess only lets a device's owner attach it). resolveDeviceClaimableOrgs widens the poll's claimable orgs to include orgs where the device has an active pin AND the owner is still a member. Keeps the device anchored to its home+personal org; revoke = unpin or leave the org.
|
Warning Review limit reached
More reviews will be available in 4 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces utilities to determine organizational scope for user-scoped device workers based on active device pins, integrates them into worker API authorization and polling flows to enable cross-organization run claiming, and adds unit and integration test coverage for the new scope logic. ChangesCross-org device pin scoping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
bug_free 78, simplicity 82, slop 6, bugs 0, 0 blockers Suite logs passed: typecheck/unit/integration all exit 0. Static-inspected worker auth, poll, and trigger paths; did not boot server. [env] Targeted rerun of the new integration test failed before execution because this shell lacks DATABASE_URL. Suggested fixes
Full verdict JSON{
"bug_free_confidence": 78,
"bugs": 0,
"slop": 6,
"simplicity": 82,
"blockers": [],
"change_type": "feat",
"behavior_change_risk": "high",
"tests_adequate": false,
"suggested_fixes": [
{
"file": "packages/server/src/__tests__/integration/auth/device-cross-org-pins.test.ts",
"line": 52,
"change": "Add a route-level poll test that seeds a cross-org device-pinned watcher or connection plus an unrelated unpinned capability run in that org, calls /api/workers/poll with the worker-bound token, and asserts only the pinned run is claimable."
},
{
"file": "packages/server/src/__tests__/integration/watchers/manual-trigger.test.ts",
"line": 320,
"change": "Add cross-org manual-trigger coverage: a worker-bound token from org A should get 200 for an org B watcher pinned to that device when the owner is still a member, and 403 after membership is removed."
}
],
"notes": "Suite logs passed: typecheck/unit/integration all exit 0. Static-inspected worker auth, poll, and trigger paths; did not boot server. [env] Targeted rerun of the new integration test failed before execution because this shell lacks DATABASE_URL.",
"categories": {
"src": 140,
"tests": 151,
"docs": 0,
"config": 0,
"deps": 0,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
…vices pi caught that widening the poll's claim scope wasn't enough: authorizeRunForWorker (complete-watcher / heartbeat) only trusted the token's base orgs or the connection-pinned device owner, so a device could CLAIM a cross-org watcher run but get 403 trying to FINISH it. Join the watcher's pinned device too and accept the owner via a shared runInWorkerScope() decision (unit-tested).
![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 (2)
packages/server/src/utils/device-claimable-orgs.ts (1)
21-24: ⚡ Quick winExtract these new object shapes into interfaces.
The new exported signatures introduce inline object-shape types. Please move them to named
interfaces so this file stays aligned with the repo's TypeScript convention.As per coding guidelines, "Use
interfacefor defining object shapes in TypeScript files".Also applies to: 55-60
🤖 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/server/src/utils/device-claimable-orgs.ts` around lines 21 - 24, Extract the inline object shapes into named exported interfaces and replace the inline types in the function signatures: create an interface (e.g., ResolveDeviceClaimableOrgsParams) representing the params object used by resolveDeviceClaimableOrgs, export it, and change the function signature to use that interface; do the same for the other exported signatures mentioned around lines 55-60 (create appropriate interface names for their parameter/return object shapes and use those interfaces in the signatures). Ensure names are descriptive, exported, and referenced where the inline object types currently appear (e.g., replace params: { deviceWorkerId: string; ownerUserId: string; baseOrgIds: string[] } with params: ResolveDeviceClaimableOrgsParams).packages/server/src/__tests__/integration/auth/device-cross-org-pins.test.ts (1)
57-106: ⚡ Quick winAdd one integration case for pinned connections too.
This suite locks in the watcher branch well, but
resolveDeviceClaimableOrgs()also widens scope fromconnections.device_worker_id. A small active/non-deleted connection-pin case would protect the other half of the SQLUNIONfrom drifting unnoticed.🤖 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/server/src/__tests__/integration/auth/device-cross-org-pins.test.ts` around lines 57 - 106, Add an integration test that covers the connections.device_worker_id branch of resolveDeviceClaimableOrgs: create a deviceWorkerId, a base org (orgA) and another org (orgD) where the user is a member, then create an active/non-deleted connection-pin record that sets connections.device_worker_id to the deviceWorkerId for orgD (use your existing helper like createTestConnection or pinConnection), call resolveDeviceClaimableOrgs with { deviceWorkerId, ownerUserId: user.id, baseOrgIds: [orgA.id] } and assert the result contains orgD (and still contains orgA); also add a small case where the connection is archived/non-active and assert it does not grant scope—this mirrors the watcher-pin tests but targets the connections.device_worker_id union branch.
🤖 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/server/src/utils/device-claimable-orgs.ts`:
- Around line 54-66: The runInWorkerScope helper currently authorizes based
solely on device_owner/watcher_device_owner, letting authorizeRunForWorker
accept cross-org runs even after membership is revoked; update runInWorkerScope
to consult the worker's current claimable orgs via resolveDeviceClaimableOrgs()
(or accept a ctx.claimableOrgIds array) and only allow the ownership fallback
when run.organization_id is still contained in that resolved set—i.e., first
check ctx.orgIds/includes(run.organization_id), then if ctx.workerUserId is
present resolve the worker's claimable org IDs and permit the
device_owner/watcher_device_owner match only when run.organization_id is in that
resolved claimable set.
---
Nitpick comments:
In
`@packages/server/src/__tests__/integration/auth/device-cross-org-pins.test.ts`:
- Around line 57-106: Add an integration test that covers the
connections.device_worker_id branch of resolveDeviceClaimableOrgs: create a
deviceWorkerId, a base org (orgA) and another org (orgD) where the user is a
member, then create an active/non-deleted connection-pin record that sets
connections.device_worker_id to the deviceWorkerId for orgD (use your existing
helper like createTestConnection or pinConnection), call
resolveDeviceClaimableOrgs with { deviceWorkerId, ownerUserId: user.id,
baseOrgIds: [orgA.id] } and assert the result contains orgD (and still contains
orgA); also add a small case where the connection is archived/non-active and
assert it does not grant scope—this mirrors the watcher-pin tests but targets
the connections.device_worker_id union branch.
In `@packages/server/src/utils/device-claimable-orgs.ts`:
- Around line 21-24: Extract the inline object shapes into named exported
interfaces and replace the inline types in the function signatures: create an
interface (e.g., ResolveDeviceClaimableOrgsParams) representing the params
object used by resolveDeviceClaimableOrgs, export it, and change the function
signature to use that interface; do the same for the other exported signatures
mentioned around lines 55-60 (create appropriate interface names for their
parameter/return object shapes and use those interfaces in the signatures).
Ensure names are descriptive, exported, and referenced where the inline object
types currently appear (e.g., replace params: { deviceWorkerId: string;
ownerUserId: string; baseOrgIds: string[] } with params:
ResolveDeviceClaimableOrgsParams).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d919b5c9-0acd-48f5-aa7c-f743b4f96cae
📒 Files selected for processing (4)
packages/server/src/__tests__/integration/auth/device-cross-org-pins.test.tspackages/server/src/__tests__/unit/run-in-worker-scope.test.tspackages/server/src/utils/device-claimable-orgs.tspackages/server/src/worker-api.ts
| export function runInWorkerScope( | ||
| run: { | ||
| organization_id: string; | ||
| device_owner: string | null; | ||
| watcher_device_owner: string | null; | ||
| }, | ||
| ctx: { workerUserId: string | null; orgIds: string[] } | ||
| ): boolean { | ||
| if (ctx.orgIds.includes(run.organization_id)) return true; | ||
| if (!ctx.workerUserId) return false; | ||
| return ( | ||
| run.device_owner === ctx.workerUserId || run.watcher_device_owner === ctx.workerUserId | ||
| ); |
There was a problem hiding this comment.
Reapply org-membership checks for owned cross-org runs.
This helper now treats device_owner / watcher_device_owner as sufficient on their own, but resolveDeviceClaimableOrgs() only grants cross-org scope while the owner is still a member. Once membership is revoked, authorizeRunForWorker() will still allow heartbeat/completion for already-running cross-org runs because this path ignores the resolved claimable-org set entirely.
[suggested fix: have completion/heartbeat authorization resolve the worker's current claimable orgs and only allow the ownership fallback when run.organization_id is still in that set.]
🤖 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/server/src/utils/device-claimable-orgs.ts` around lines 54 - 66, The
runInWorkerScope helper currently authorizes based solely on
device_owner/watcher_device_owner, letting authorizeRunForWorker accept
cross-org runs even after membership is revoked; update runInWorkerScope to
consult the worker's current claimable orgs via resolveDeviceClaimableOrgs() (or
accept a ctx.claimableOrgIds array) and only allow the ownership fallback when
run.organization_id is still contained in that resolved set—i.e., first check
ctx.orgIds/includes(run.organization_id), then if ctx.workerUserId is present
resolve the worker's claimable org IDs and permit the
device_owner/watcher_device_owner match only when run.organization_id is in that
resolved claimable set.
pi: the widened claimableOrgIds was shared by all claim branches, so one pin in org B also let the device claim unrelated unpinned capability-matched device runs in org B. Split scopes: pinned branches (connection/watcher) keep the widened, membership-gated scope; the unpinned capability branch uses the base [bound, personal] scope only.
Third endpoint with the same gap (pi): /api/workers/me/watchers/:id/trigger was base-org scoped, so 'Run now' on a cross-org pinned watcher 403'd. Accept the watcher's org when the caller is still a member; the existing pin-to-this-device check is the consent. poll + complete + trigger are now consistent.
|
pi review (4 rounds, adversarial): caught + fixed 3 real cross-org consistency bugs — completion/heartbeat auth (403 after claim), over-broad scope (one pin exposed unpinned capability runs), and the manual-trigger gap — plus a redeclare. Final verdict: bug_free 78, simplicity 82, 0 bugs, 0 blockers, typecheck/unit/integration all green. Core logic covered by a unit test (runInWorkerScope) + integration test (resolveDeviceClaimableOrgs). Follow-up (deferred): route-level /api/workers/poll + manual-trigger endpoint tests for the pinned-vs-unpinned scope split. |
2 participants
Gap
A user-scoped device worker (e.g. Owletto Mac) could only claim runs in
[token's bound org, personal org](index.ts:766). So a watcher/connection pinned to the device in another org the owner belongs to was never claimable — the run satpendingforever (the server deliberately skips device-pinned runs). This blocked running an org's watcher on a personal device via local Claude.Fix
resolveDeviceClaimableOrgswidens the poll's claimable orgs to include any org where the device has an active pin AND its owner is still a member. The pin is already the consent (evaluateDeviceWorkerAccessonly lets a device's owner attach it); the membership join revokes scope automatically if the owner leaves. The device stays anchored to its home+personal org; within-org claiming still follows the existing pinned/capability rules, so it only runs the resource it was pinned to. Revoke = unpin.Tests
New integration test (2 cases): pinned+member org included, pinned+non-member excluded, base scope preserved, archived-watcher pin grants nothing. Passes locally under Node 22 + embedded PG.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Tests