fix(cli): stabilize delegated credentials#294
Conversation
![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 several new subcommands to the CLI, including writeback push for uploading local files, writeback skip-stuck to bypass unreadable events, and workspace status to report workspace health. It also refactors delegated credentials storage to be workspace- and scope-specific, and adds a cloud re-minting fallback for expired tokens. Two critical issues were identified in the review: first, the direct use of syscall.Flock will break compilation on Windows, requiring a cross-platform file locking solution; second, a race condition exists in SkipStuck where releasing the mutex before calling s.sync could allow concurrent operations to run with skipStuckMode enabled, potentially polluting the syncer's state.
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.
| if err := syscall.Flock(int(file.Fd()), syscall.LOCK_EX); err != nil { | ||
| return err | ||
| } | ||
| defer syscall.Flock(int(file.Fd()), syscall.LOCK_UN) |
There was a problem hiding this comment.
Using syscall.Flock directly will cause compilation to fail on Windows because syscall.Flock, syscall.LOCK_EX, and syscall.LOCK_UN are not defined on Windows. Since this CLI is intended to support Windows (as evidenced by other platform-specific path handling and comments), you should use a cross-platform file locking library such as github.com/gofrs/flock, or split the locking implementation into platform-specific files using build tags (e.g., lock_unix.go and lock_windows.go).
| func (s *Syncer) SkipStuck(ctx context.Context, max int) (int, error) { | ||
| s.mu.Lock() | ||
| s.skipStuckMode = true | ||
| s.skipStuckMax = max | ||
| s.skipStuckCount = 0 | ||
| s.mu.Unlock() | ||
| defer func() { | ||
| s.mu.Lock() | ||
| s.skipStuckMode = false | ||
| s.skipStuckMax = 0 | ||
| s.mu.Unlock() | ||
| }() | ||
| err := s.sync(ctx, true) | ||
| s.mu.Lock() | ||
| count := s.skipStuckCount | ||
| s.mu.Unlock() | ||
| return count, err | ||
| } |
There was a problem hiding this comment.
There is a race condition and state pollution risk in SkipStuck. The mutex s.mu is unlocked before calling s.sync(ctx, true) to avoid a deadlock (since s.sync locks s.mu internally). However, during this unlocked window, another concurrent goroutine could call Reconcile or another method that triggers s.sync, which would then run with s.skipStuckMode = true and pollute the state.\n\nTo resolve this, consider refactoring s.sync into an unexported s.syncLocked method that assumes s.mu is already held. Then, s.sync can simply lock s.mu and call s.syncLocked. This allows SkipStuck to keep s.mu locked for the entire duration of the operation, ensuring atomicity and preventing concurrent state pollution.
![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: f054d2b15b
ℹ️ 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".
| if strings.TrimSpace(workspaceValue) != "" || len(scopes) > 0 { | ||
| return delegatedCredentialsPathForRequest(workspaceValue, scopes), false |
There was a problem hiding this comment.
When callers omit --workspace but pass any scope set (for example mount passes defaultJoinScopes), this branch hashes the literal empty/active workspace key. Login/setup/join save delegated bundles with delegatedCredentialsPathForRequest(record.ID, ...), so a normal no-arg relayfile mount after login looks in a different path and fails with missing delegated credentials instead of using the active/default workspace. Resolve the empty workspace value to the active/default workspace ID before deriving the scoped credential path.
Useful? React with 👍 / 👎.
| CorrelationID: commandID, | ||
| ContentIdentity: identity, | ||
| } | ||
| if err := writeWritebackPushReceipt(resolved.MountRoot, pendingReceipt); err != nil { |
There was a problem hiding this comment.
This writes a pending outbox receipt before the direct /fs/bulk call finishes. A running mount daemon can read that pending file while the command is waiting for the writeback op, but internal/mountsync/outbox.go drops the receipt's contentIdentity and re-sends it as a mount-command keyed by commandId, while the direct push uses mount-writeback-create-draft keyed by workspace/path/hash. If the daemon flushes during the wait, the server sees two different dedupe keys and can create duplicate provider drafts/tickets; either keep the pending record private until retry is needed or have mountsync preserve this same content identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e16f30f. The durable-outbox flush (outboxRecordsAsBulkFiles) now derives the mount-writeback-create-draft content identity (workspace:path:hash) for draft paths via outboxRecordContentIdentity, matching both the in-flight bulkWriteFilesForPending path and the CLI direct push — so a daemon flush of a pending push receipt collides with the direct push instead of minting a second idempotency key. Non-draft mount commands keep the stable commandId identity. Also fixes a latent inconsistency where a daemon-originated draft used the draft identity on first attempt but commandId on persisted-outbox retry. Updated TestBulkWrite_FlushSendsContentIdentity (it had encoded the buggy mount-command assertion for a draft path) and added TestOutboxRecordContentIdentityDraftMatchesDirectPush.
khaliqgant
force-pushed
the
pear/workforce-e3c9d114
branch
from
f054d2b to
6f0fb19
Compare
|
Warning Review limit reached
More reviews will be available in 35 minutes and 40 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 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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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 ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds three new CLI subcommands ( ChangesSyncer stuck-event infrastructure, scoped delegated credentials, writeback push, and workspace status
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
note over CLI,BulkWriteAPI: relayfile writeback push LOCAL_PATH
end
participant CLI
participant loadDelegatedCredentialsForRequest
participant bootstrapDelegatedCredentialsFromAgentRelay
participant BulkWriteAPI
participant OpStatusAPI
CLI->>loadDelegatedCredentialsForRequest: workspace + scopes
alt no scoped bundle
loadDelegatedCredentialsForRequest->>bootstrapDelegatedCredentialsFromAgentRelay: bootstrap from agent-relay
bootstrapDelegatedCredentialsFromAgentRelay-->>loadDelegatedCredentialsForRequest: scoped creds path + bundle
end
loadDelegatedCredentialsForRequest-->>CLI: delegated bundle
CLI->>CLI: encode file, detect content type, SHA256 hash, compute contentIdentity
CLI->>CLI: write pending receipt
CLI->>BulkWriteAPI: POST bulk FS write (content + contentIdentity)
alt bulk write error
BulkWriteAPI-->>CLI: error
CLI->>CLI: write failed receipt (body dropped)
else bulk write success + opId
BulkWriteAPI-->>CLI: opId
loop poll until terminal
CLI->>OpStatusAPI: GET op status
OpStatusAPI-->>CLI: status
end
CLI->>CLI: write acked receipt (body dropped) or needsAttention
end
sequenceDiagram
rect rgba(100, 160, 100, 0.5)
note over CLI,Syncer: relayfile writeback skip-stuck
end
participant CLI
participant runWritebackSkipStuck
participant Syncer
CLI->>runWritebackSkipStuck: max (optional)
runWritebackSkipStuck->>Syncer: SkipStuck(ctx, max)
Syncer->>Syncer: set skipStuckMode, run reconcile
loop unreadable 404 events up to max
Syncer->>Syncer: drop event, advance incremental checkpoint
end
Syncer-->>runWritebackSkipStuck: skipped count
runWritebackSkipStuck->>Syncer: BacklogDraining()
Syncer-->>runWritebackSkipStuck: draining bool
runWritebackSkipStuck-->>CLI: report skipped / backlog / error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/relayfile-cli/main.go (1)
1680-1703:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve catalog scopes when minting command-scoped bundles.
This uses the requested mint scopes as
record.Scopes, then persists them viapersistDelegatedWorkspace. Awriteback pushbootstrap with provider-specific scopes can overwrite the workspace’s default catalog scopes, so later default refreshes may lose the originalfs:read/fs:writeset.Proposed direction
- recordScopes := append([]string(nil), scopes...) - if len(recordScopes) == 0 { - recordScopes = append([]string(nil), defaultJoinScopes...) + mintScopes := append([]string(nil), scopes...) + if len(mintScopes) == 0 { + mintScopes = append([]string(nil), defaultJoinScopes...) + } + recordScopes := append([]string(nil), mintScopes...) + if existing, ok := workspaceRecordByID(cloudWorkspaceID); ok && len(existing.Scopes) > 0 { + recordScopes = append([]string(nil), existing.Scopes...) + } else if relayID := strings.TrimSpace(workspace.RelayfileWorkspaceID); relayID != "" { + if existing, ok := workspaceRecordByID(relayID); ok && len(existing.Scopes) > 0 { + recordScopes = append([]string(nil), existing.Scopes...) + } } @@ - delegated, err := delegatedRelayfileTokenViaCloud(cloudCreds, cloudWorkspaceID, record.AgentName, record.Scopes) + delegated, err := delegatedRelayfileTokenViaCloud(cloudCreds, cloudWorkspaceID, record.AgentName, mintScopes) @@ - credsPath := delegatedCredentialsPathForRequest(record.ID, record.Scopes) + credsPath := delegatedCredentialsPathForRequest(record.ID, mintScopes)🤖 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 1680 - 1703, The current implementation uses the command-specific scopes (from the mint request) to set record.Scopes, which gets persisted via persistDelegatedWorkspace, causing the original catalog scopes like fs:read/fs:write to be overwritten and lost in later default refreshes. To fix this, preserve the original default catalog scopes by keeping them in record.Scopes instead of replacing them with the command-specific scopes, and pass the command-scoped scopes as a separate parameter to persistDelegatedWorkspace so the function can handle both the persistent catalog scopes and the command-specific scopes appropriately.
🤖 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.go`:
- Around line 6408-6415: In the resolveDelegatedCredentialsPathForRequest
function, resolve the active workspace to its concrete workspace name before
checking for scoped credentials. Currently, if workspaceValue is "active" or
empty, it gets passed literally to delegatedCredentialsPathForRequest, causing
the function to miss the correct scoped credentials file that was saved under
the concrete workspace key. Add a workspace resolution step (likely by calling a
function that converts "active" to the actual workspace name) before the
condition that checks if workspaceValue is not empty or scopes exist, ensuring
scoped credential lookup uses the resolved workspace name instead of the literal
"active" value.
- Around line 3788-3798: The code currently returns record.ID for workspace
identification, but for delegated workspaces this can be the Cloud workspace ID
rather than the Relayfile runtime workspace ID. In both the
workspaceRecordByName and workspaceRecordByID branches, modify the return
statements to prioritize returning local.RelayWorkspaceID when it is available
(not empty), and only fall back to using local.ID if RelayWorkspaceID is empty.
This ensures the correct workspace ID is returned to consumers like writeback
skip-stuck that need to instantiate the syncer against the proper workspace.
- Around line 4655-4672: The readLocalMountCursorHealth function returns
immediately after finding and parsing the first readable file, which prevents
aggregation of health data from multiple locations. Instead of returning early
from the loop when a valid state file is found, collect the stuck count and
backlog status from all readable files at different paths, then return the
maximum stuck count across all files and the OR of all backlog flags to ensure
no stuck backlog state is hidden by an older file in an earlier location.
- Around line 3182-3200: The delegatedBundleHasScopes function uses exact string
comparison (strings.TrimSpace(got) == want) which fails to handle wildcard
patterns in scopes. Replace the exact string equality check in the inner for
loop with a call to a wildcard-aware scope matcher function that can properly
evaluate whether a scope like relayfile:fs:write:/** satisfies a requirement for
relayfile:fs:write:/github/**. This will prevent unnecessary re-minting and
allow writeback push operations when the bundle has a sufficiently broad scope
grant.
- Around line 3491-3502: The skip-stuck functionality currently requires legacy
credentials.json through the loadCredentials call, which causes delegated-only
users (who use Agent Relay login without credentials.json) to fail before
SkipStuck can run. Modify the code block starting with loadCredentials() to
support delegated credentials as an alternative path: instead of returning an
error when credentials are not found, implement fallback logic to resolve the
token and server through delegated authentication (Agent Relay), and only fail
if both the legacy credentials path and delegated path fail to provide the
required token. This allows users without credentials.json to successfully use
the skip-stuck command through their delegated Agent Relay login.
---
Outside diff comments:
In `@cmd/relayfile-cli/main.go`:
- Around line 1680-1703: The current implementation uses the command-specific
scopes (from the mint request) to set record.Scopes, which gets persisted via
persistDelegatedWorkspace, causing the original catalog scopes like
fs:read/fs:write to be overwritten and lost in later default refreshes. To fix
this, preserve the original default catalog scopes by keeping them in
record.Scopes instead of replacing them with the command-specific scopes, and
pass the command-scoped scopes as a separate parameter to
persistDelegatedWorkspace so the function can handle both the persistent catalog
scopes and the command-specific scopes appropriately.
🪄 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: 3e389bb2-a447-49e4-9f12-e6358176b863
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
cmd/relayfile-cli/main.gocmd/relayfile-cli/main_test.gointernal/mountsync/syncer.gointernal/mountsync/syncer_test.go
khaliqgant
force-pushed
the
pear/workforce-e3c9d114
branch
from
6f0fb19 to
c501079
Compare
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. |
…ntity A direct `relayfile writeback push` deposits a pending receipt into the shared `.relay/outbox/pending/` dir before its synchronous bulk write + op-status poll completes. A running mount daemon can flush that pending receipt during the wait. The durable-outbox flush keyed every record on the per-record commandId (`mount-command`), while the direct push — and the in-flight `bulkWriteFilesForPending` path — key draft creates on (workspace, path, content hash) (`mount-writeback-create-draft`). The two different idempotency keys let the server mint duplicate provider drafts/tickets for identical content. Make `outboxRecordsAsBulkFiles` derive the create-draft identity for draft paths via `outboxRecordContentIdentity`, matching the in-flight path and the CLI direct push. Non-draft mount commands keep the stable commandId identity. This also closes a latent inconsistency: a daemon-originated draft write used the create-draft identity on its first attempt but switched to the commandId identity on persisted-outbox retry. Addresses Codex P1 review on PR #294. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review triage (commit e16f30f)No merge conflicts — the branch is up to date with Fixed now
Already addressed in earlier commits on this branch (verified against current code)
Validation: |
1 participant
Summary
Fixes #291
Validation
Notes