Use delegated relayfile token refresh#274
Merged
Merged
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 48 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR replaces Cloud "join" tokens with delegated relayfile credentials minted by Cloud, persisted locally, and refreshed via HTTP token endpoints. Delegated credentials flow through setup, workspace join, integration operations, and mount-loop operations to eliminate join-based token bootstrapping and improve credential lifecycle management. ChangesDelegated Credentials Implementation
Sequence Diagram(s)sequenceDiagram
participant Setup
participant Cloud API
participant delegatedauth
participant LocalDisk
participant Mount
Setup->>Cloud API: POST /relayfile/delegated-token
Cloud API-->>Setup: Bearer + Rotation tokens, refresh URL
Setup->>delegatedauth: SaveAtomic(delegated_creds_path, Bundle)
delegatedauth->>LocalDisk: write JSON with atomic temp
Setup->>Mount: --server <from bundle> --creds-file <path>
Mount->>delegatedauth: Load(delegated_creds_path)
delegatedauth->>LocalDisk: read JSON
delegatedauth-->>Mount: Bundle
Mount->>delegatedauth: Renew(ctx, client, bundle, timeout)
delegatedauth->>Cloud API: POST /v1/tokens/refresh
Cloud API-->>delegatedauth: new Bearer + Rotation tokens
delegatedauth->>LocalDisk: SaveAtomic with rotated tokens
Mount->>Mount: sync data-plane with refreshed bearer
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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 |
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
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. |
khaliqgant
force-pushed
the
auth-unify-direct-http-2108
branch
from
27583cb to
e12a638
Compare
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@cmd/relayfile-cli/main_test.go`:
- Around line 285-291: Tests mutate shared flags from httptest handlers (e.g.
the boolean seenBootstrap) without synchronization; convert each such
flag/counter used by handlers and later assertions to an atomic-backed type
(e.g. change seenBootstrap bool -> seenBootstrap int32) and use
sync/atomic.StoreInt32 in the handler and sync/atomic.LoadInt32 in the test
assertions (or protect access with a sync.Mutex if you prefer). Apply the same
change to the other handler-set counters/flags referenced in the review so all
writes in the httptest handlers are synchronized with the reads in the test
code.
In `@cmd/relayfile-cli/main.go`:
- Around line 4397-4400: The code incorrectly returns the stale auth error `err`
after calling `c.refreshFromDelegated()` instead of the new delegated-refresh
error; change the error return so that when `refreshFromDelegated()` returns a
non-nil `refreshErr` you return that `refreshErr` (not `err`) (i.e., replace the
current `return body, contentType, err` with `return body, contentType,
refreshErr`), and make the identical fix at the other occurrence (lines
4408-4411) where `c.refreshFromDelegated()` is checked; this ensures callers
like `runStatus` and other workspace verbs receive the delegated-expiry error
from `c.refreshFromDelegated()` rather than the stale 401/403.
- Around line 5433-5436: The current code treats every
delegatedauth.ErrRefreshRejected as a permanent credential expiry; instead, only
map to ErrDelegatedRelayfileCredentialsExpired when the delegatedauth error
indicates a permanent revocation (e.g., error code or sentinel for
delegation_expired or workspace_token_revoked). Modify the error handling around
the existing if errors.Is(err, delegatedauth.ErrRefreshRejected) check: inspect
the concrete delegatedauth error (unwrap err and check its Code/Type or use a
provided helper like delegatedauth.IsPermanentRevocation(err)) and return
ErrDelegatedRelayfileCredentialsExpired only for those permanent-revocation
cases; for all other refresh-rejected cases (transient timeouts, non-2xx, etc.)
return the original err (or wrap it without converting to
ErrDelegatedRelayfileCredentialsExpired) so the mount is not forced into
degraded mode. Ensure you reference the same variables (err, bundle) and the
same sentinels (delegatedauth.ErrRefreshRejected,
ErrDelegatedRelayfileCredentialsExpired) when making this change.
- Around line 1619-1622: In persistDelegatedWorkspace (cmd/relayfile-cli/main.go
lines 1619-1622) stop clearing record.CloudAPIURL — remove the assignment that
sets it to "" so the existing workspace Cloud API URL is preserved; for each
other site (cmd/relayfile-cli/main.go:2041-2041, 2394-2395, 2455-2456,
2528-2529, 2673-2674, 3428-3429) where the code builds a Cloud client or chooses
a cloud API URL (e.g. in the integration
connect/list/disconnect/adopt/set-metadata and ops replay paths), change the
logic that currently defaults straight to the global/default URL to instead
first use the saved workspace record.CloudAPIURL when non-empty, then fall back
to the flag/env default — i.e., prefer workspaceRecord.CloudAPIURL ||
providedFlag || default.
- Around line 5395-5410: The global delegated-credentials.json must be replaced
with a workspace-scoped path or a persisted per-workspace field; change
delegatedCredentialsPath/resolveDelegatedCredentialsPath (functions
delegatedCredentialsPath and resolveDelegatedCredentialsPath) to return a
workspace-specific filename (e.g. include workspace ID/name) or stop using a
global helper and instead store/load the delegated credentials path in the
workspaceRecord. For each affected site apply these concrete edits:
cmd/relayfile-cli/main.go (lines 5395-5410) — modify
resolveDelegatedCredentialsPath/delegatedCredentialsPath to accept or derive a
workspace identifier and return a workspace-scoped path (or remove global helper
and document workspaceRecord field); cmd/relayfile-cli/main.go (lines 847-849) —
when saving setup-minted delegated bundles, write to the workspace-specific path
(persist path into workspaceRecord if using that approach);
cmd/relayfile-cli/main.go (lines 2034-2035) — write integration-connect
bootstrap bundles to the workspace-scoped path; cmd/relayfile-cli/main.go (lines
3624-3625) — write workspace join bootstrapped bundles to the workspace-scoped
path; cmd/relayfile-cli/main.go (lines 3907-3923) — resolve/load the delegated
bundle from the selected workspace’s stored path (not the global file);
cmd/relayfile-cli/main.go (lines 4235-4258) — update
prepareWorkspaceCommandClient to load the delegated bundle from the
workspace-specific path (or workspaceRecord) so status/tree/read/export operate
per-workspace. Ensure all callers use the new API or workspaceRecord field so no
code reads/writes the old global file.
- Around line 1601-1605: The code currently backfills
bundle.RelayfileWorkspaceID with the cloud control-plane workspaceID when
bundle.Workspace() is empty; instead, change this to fail fast: if
bundle.Workspace() == "" return a descriptive error (e.g., "missing relayfile
workspace id") rather than assigning workspaceID, so ValidateForUse isn't
misled; update the error path before calling bundle.ValidateForUse() and remove
the assignment to RelayfileWorkspaceID so the runtime Relayfile workspace id
must be explicitly provided.
In `@cmd/relayfile-mount/main.go`:
- Line 513: The bearer-token precedence is inverted: change the token selection
to prefer creds.AccessToken before creds.Token so it matches
delegatedauth.Bundle.BearerToken()'s behavior; replace the call that sets token
(currently firstNonEmpty(creds.Token, creds.AccessToken, creds.RelayfileToken))
with firstNonEmpty(creds.AccessToken, creds.Token, creds.RelayfileToken) (or
otherwise call into the same Bundle.BearerToken() helper) so startup uses the
same precedence as internal/delegatedauth/delegatedauth.go:74-76.
In `@internal/delegatedauth/delegatedauth.go`:
- Around line 236-287: The Renew() flow currently maps transport timeouts and
all non-2xx HTTP responses to ErrRefreshRejected; change Renew() so client.Do
errors (including context.DeadlineExceeded/timeouts) return the original error
(or a wrapped non-ErrRefreshRejected error) instead of wrapping with
ErrRefreshRejected, and change refreshHTTPError(status,payload) so only explicit
delegation errors (parsed.Code == "delegation_expired" or
"workspace_token_revoked") return ErrRefreshRejected while all other HTTP
failures return a generic refresh error that does NOT wrap ErrRefreshRejected;
update/enable regression tests to assert that a 5xx response and a request
timeout do NOT produce ErrRefreshRejected but do return an error indicating
refresh failed (i.e., add tests exercising Renew() and refreshHTTPError for
timeout and 5xx cases).
- Around line 118-128: Validate the relayfile URL in Bundle.ValidateForUse by
parsing b.ServerURL() (use net/url.Parse or url.ParseRequestURI) and ensure it
is an absolute URL with a non-empty Scheme and Host (and optionally enforce
http/https if desired); if parsing fails or required parts are missing return a
descriptive error like "delegated relayfile credentials invalid relayfileUrl"
instead of accepting any non-empty string so malformed values (e.g.,
"relayfile") fail fast; update Bundle.ValidateForUse to perform this URL
validation alongside the existing BearerToken() and Workspace() checks.
🪄 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: 9bfd888d-f495-48c6-aec0-e1e93267456d
📒 Files selected for processing (9)
cmd/relayfile-cli/background_test.gocmd/relayfile-cli/main.gocmd/relayfile-cli/main_test.gocmd/relayfile-cli/productized_cloud_mount_e2e_test.gocmd/relayfile-cli/setup_e2e_test.gocmd/relayfile-cli/writeback_daemon_test.gocmd/relayfile-mount/main.gointernal/delegatedauth/delegatedauth.gointernal/delegatedauth/delegatedauth_test.go
Comment on lines
+285
to
291
| var seenBootstrap bool | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Path != "/api/v1/workspaces/ws_cloud/join" { | ||
| if r.URL.Path != "/api/v1/workspaces/ws_cloud/relayfile/delegated-token" { | ||
| t.Fatalf("unexpected path: %s", r.URL.Path) | ||
| } | ||
| seenJoin = true | ||
| seenBootstrap = true | ||
| if got := r.Header.Get("Authorization"); got != "Bearer cld_access" { |
There was a problem hiding this comment.
Guard handler-shared state with atomics/mutex to keep these tests race-safe.
httptest handlers run on separate goroutines. Writes on Line 290, Line 413, Line 526, Line 2085, and Line 2173 (same pattern in nearby changed counters/flags) are read later without synchronization, which can fail under go test -race and make assertions flaky.
🔧 Suggested fix pattern
- var seenBootstrap bool
+ var seenBootstrap atomic.Bool
- seenBootstrap = true
+ seenBootstrap.Store(true)
- if !seenBootstrap {
+ if !seenBootstrap.Load() {
t.Fatalf("expected delegated-token bootstrap call")
}
- var refreshCount int
- var treeCount int
+ var refreshCount atomic.Int32
+ var treeCount atomic.Int32
- refreshCount++
+ refreshCount.Add(1)
- treeCount++
+ treeCount.Add(1)
- if refreshCount != 1 || treeCount != 1 {
- t.Fatalf("refreshCount/treeCount = %d/%d, want 1/1", refreshCount, treeCount)
+ if refreshCount.Load() != 1 || treeCount.Load() != 1 {
+ t.Fatalf("refreshCount/treeCount = %d/%d, want 1/1", refreshCount.Load(), treeCount.Load())
}Also applies to: 407-414, 520-527, 2077-2086, 2166-2174
🤖 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 `@cmd/relayfile-cli/main_test.go` around lines 285 - 291, Tests mutate shared
flags from httptest handlers (e.g. the boolean seenBootstrap) without
synchronization; convert each such flag/counter used by handlers and later
assertions to an atomic-backed type (e.g. change seenBootstrap bool ->
seenBootstrap int32) and use sync/atomic.StoreInt32 in the handler and
sync/atomic.LoadInt32 in the test assertions (or protect access with a
sync.Mutex if you prefer). Apply the same change to the other handler-set
counters/flags referenced in the review so all writes in the httptest handlers
are synchronized with the reads in the test code.
Comment on lines
+1601
to
+1605
| if bundle.Workspace() == "" { | ||
| bundle.RelayfileWorkspaceID = workspaceID | ||
| } | ||
| if strings.TrimSpace(joined.Token) == "" { | ||
| return cloudWorkspaceJoinResponse{}, errors.New("cloud join response missing token") | ||
| if err := bundle.ValidateForUse(); err != nil { | ||
| return delegatedauth.Bundle{}, fmt.Errorf("delegated relayfile credential bundle invalid: %w", err) |
There was a problem hiding this comment.
Fail when relayfileWorkspaceId is missing instead of substituting the Cloud workspace id.
workspaceID here is the Cloud control-plane id, but the rest of this file already treats the Relayfile runtime id as a separate field (RelayWorkspaceID). Backfilling a missing runtime id with the Cloud id makes ValidateForUse pass and can send later data-plane calls to the wrong workspace on deployments where those ids differ.
Suggested fix
- if bundle.Workspace() == "" {
- bundle.RelayfileWorkspaceID = workspaceID
- }
+ if bundle.Workspace() == "" {
+ return delegatedauth.Bundle{}, errors.New("delegated relayfile credential bundle missing relayfileWorkspaceId")
+ }📝 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.
Suggested change
| if bundle.Workspace() == "" { | |
| bundle.RelayfileWorkspaceID = workspaceID | |
| } | |
| if strings.TrimSpace(joined.Token) == "" { | |
| return cloudWorkspaceJoinResponse{}, errors.New("cloud join response missing token") | |
| if err := bundle.ValidateForUse(); err != nil { | |
| return delegatedauth.Bundle{}, fmt.Errorf("delegated relayfile credential bundle invalid: %w", err) | |
| if bundle.Workspace() == "" { | |
| return delegatedauth.Bundle{}, errors.New("delegated relayfile credential bundle missing relayfileWorkspaceId") | |
| } | |
| if err := bundle.ValidateForUse(); err != nil { | |
| return delegatedauth.Bundle{}, fmt.Errorf("delegated relayfile credential bundle invalid: %w", err) |
🤖 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 `@cmd/relayfile-cli/main.go` around lines 1601 - 1605, The code currently
backfills bundle.RelayfileWorkspaceID with the cloud control-plane workspaceID
when bundle.Workspace() is empty; instead, change this to fail fast: if
bundle.Workspace() == "" return a descriptive error (e.g., "missing relayfile
workspace id") rather than assigning workspaceID, so ValidateForUse isn't
misled; update the error path before calling bundle.ValidateForUse() and remove
the assignment to RelayfileWorkspaceID so the runtime Relayfile workspace id
must be explicitly provided.
Comment on lines
+1619
to
+1622
| record.Server = strings.TrimRight(bundle.ServerURL(), "/") | ||
| record.LocalDir = localDir | ||
| record.CloudAPIURL = "" | ||
| record.LastUsedAt = time.Now().UTC().Format(time.RFC3339) |
There was a problem hiding this comment.
The delegated bootstrap path drops the workspace's Cloud control-plane URL. persistDelegatedWorkspace clears record.CloudAPIURL, while the integration and ops verbs below rebuild their Cloud clients from flag/env defaults only. After bootstrapping against a non-default Cloud API, those commands will start talking to https://agentrelay.com/cloud unless the operator repeats --cloud-api-url on every invocation.
cmd/relayfile-cli/main.go#L1619-L1622: preserverecord.CloudAPIURLduring delegated persistence instead of blanking it.cmd/relayfile-cli/main.go#L2041-L2041: when connecting integrations, fall back to the workspace's saved Cloud API URL before defaulting.cmd/relayfile-cli/main.go#L2394-L2395: do the same forintegration list.cmd/relayfile-cli/main.go#L2455-L2456: do the same forintegration disconnect.cmd/relayfile-cli/main.go#L2528-L2529: do the same forintegration adopt.cmd/relayfile-cli/main.go#L2673-L2674: do the same forintegration set-metadata.cmd/relayfile-cli/main.go#L3428-L3429: do the same forops replay.
📍 Affects 1 file
cmd/relayfile-cli/main.go#L1619-L1622(this comment)cmd/relayfile-cli/main.go#L2041-L2041cmd/relayfile-cli/main.go#L2394-L2395cmd/relayfile-cli/main.go#L2455-L2456cmd/relayfile-cli/main.go#L2528-L2529cmd/relayfile-cli/main.go#L2673-L2674cmd/relayfile-cli/main.go#L3428-L3429
🤖 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 `@cmd/relayfile-cli/main.go` around lines 1619 - 1622, In
persistDelegatedWorkspace (cmd/relayfile-cli/main.go lines 1619-1622) stop
clearing record.CloudAPIURL — remove the assignment that sets it to "" so the
existing workspace Cloud API URL is preserved; for each other site
(cmd/relayfile-cli/main.go:2041-2041, 2394-2395, 2455-2456, 2528-2529,
2673-2674, 3428-3429) where the code builds a Cloud client or chooses a cloud
API URL (e.g. in the integration connect/list/disconnect/adopt/set-metadata and
ops replay paths), change the logic that currently defaults straight to the
global/default URL to instead first use the saved workspace record.CloudAPIURL
when non-empty, then fall back to the flag/env default — i.e., prefer
workspaceRecord.CloudAPIURL || providedFlag || default.
Comment on lines
+4397
to
4400
| if refreshErr := c.refreshFromDelegated(); refreshErr != nil { | ||
| return body, contentType, err | ||
| } | ||
| return c.client.getBytes(ctx, pathForWorkspace(c.workspaceID)) |
There was a problem hiding this comment.
Return the delegated-refresh error, not the stale auth error.
Both retry helpers drop refreshFromDelegated() failures and return the original 401/403 instead. That loses the new delegated-expiry signal added in this PR, so callers like runStatus can misclassify a revoked delegation as a Cloud-session expiry and the other workspace verbs surface a generic auth error.
Suggested fix
if refreshErr := c.refreshFromDelegated(); refreshErr != nil {
- return body, contentType, err
+ return body, contentType, refreshErr
} if refreshErr := c.refreshFromDelegated(); refreshErr != nil {
- return err
+ return refreshErr
}Also applies to: 4408-4411
🤖 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 `@cmd/relayfile-cli/main.go` around lines 4397 - 4400, The code incorrectly
returns the stale auth error `err` after calling `c.refreshFromDelegated()`
instead of the new delegated-refresh error; change the error return so that when
`refreshFromDelegated()` returns a non-nil `refreshErr` you return that
`refreshErr` (not `err`) (i.e., replace the current `return body, contentType,
err` with `return body, contentType, refreshErr`), and make the identical fix at
the other occurrence (lines 4408-4411) where `c.refreshFromDelegated()` is
checked; this ensures callers like `runStatus` and other workspace verbs receive
the delegated-expiry error from `c.refreshFromDelegated()` rather than the stale
401/403.
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
1 similar comment
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
Contributor
|
No code edits were needed after validating the current checkout. The delegated-auth PR surface already passes the relevant behavior and stale legacy Addressed comments
Advisory NotesThe remaining historical bot artifacts target mountfuse, core webhooks, or an old SDK package path outside ValidationPassed locally:
|
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
1 similar comment
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
Contributor
|
Implemented fixes for the validated PR review findings and re-ran the CI-equivalent checks locally. Changes made:
Addressed comments
Advisory Notes
Validation passed:
|
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
2 similar comments
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
Contributor
|
pr-reviewer could not complete review for #274 in AgentWorkforce/relayfile. |
This was referenced Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Notes