fix: close relayfile mount conflict and lifecycle gaps - #393
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (3)
📝 WalkthroughWalkthroughThe CLI adds legacy credential fallback, canonical workspace resolution, duplicate-name disambiguation, and improved daemon discovery. Mount synchronization now rejects stale reads, preserves local edits, and records conflicts for concurrent divergence. ChangesCLI credential, workspace, and daemon behavior
Mount synchronization conflict handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Credentials
participant Catalog
participant RelayfileAPI
CLI->>Credentials: Load delegated credentials
Credentials-->>CLI: Return delegated or legacy token
CLI->>Catalog: Resolve workspace ID
Catalog-->>CLI: Return canonical workspace record
CLI->>RelayfileAPI: Send authenticated request
sequenceDiagram
participant WebSocket
participant Syncer
participant LocalWatcher
participant FileState
WebSocket->>Syncer: Deliver remote file event
Syncer->>FileState: Snapshot tracked state
Syncer->>WebSocket: Read remote file
LocalWatcher->>FileState: Advance state after local write
Syncer->>FileState: Discard stale response or apply conflict result
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 |
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. |
3b9bc4e to
de42a9c
Compare
|
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: de42a9c7fc
ℹ️ 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 berr != nil { | ||
| return fmt.Errorf("resolve delegated relayfile credentials: %w", berr) | ||
| } | ||
| delegatedCredsPath = path | ||
| bundle, berr = refreshDelegatedCredentials(path, bundle, false) | ||
| if berr != nil { | ||
| return fmt.Errorf("refresh delegated relayfile credentials: %w", berr) | ||
| } | ||
| canonicalWorkspaceID = bundle.Workspace() | ||
| if requestedWorkspace != "" && !workspaceRequestMatchesDelegatedCredentials(requestedWorkspace, canonicalWorkspaceID) { | ||
| return fmt.Errorf( | ||
| "relayfile mount without --token uses delegated relayfile workspace %s; pass --token for explicit workspace %q or re-bootstrap delegated credentials for that workspace", | ||
| canonicalWorkspaceID, | ||
| requestedWorkspace, | ||
| ) | ||
| tokenValue = strings.TrimSpace(creds.Token) |
There was a problem hiding this comment.
When --creds-file or RELAYFILE_MOUNT_CREDS_FILE explicitly selects a missing, malformed, or otherwise unusable delegated bundle and saved login credentials also exist, this branch silently discards the delegated-credential error and mounts with the saved token instead. This violates the explicit credential selection (the standalone mount command treats the credentials file as taking precedence and fails if it cannot be read) and can send setup/mount requests using a token from another workspace or server; only use this fallback when no delegated credentials path was explicitly supplied.
Useful? React with 👍 / 👎.
![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 (5)
cmd/relayfile-cli/main_test.go (1)
2269-2281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate
HOMEand Relayfile environment variables in this test.The neighboring tests call
t.Setenv("HOME", t.TempDir())andclearRelayfileEnv(t). This test omits both.runningMountDaemonscan read state outside the temp directory, so the result may depend on the developer machine or CI image.♻️ Proposed change
func TestRunningMountDaemonsExcludesCallerPIDFromPIDFile(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + clearRelayfileEnv(t) localDir := t.TempDir()🤖 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 2269 - 2281, Update TestRunningMountDaemonsExcludesCallerPIDFromPIDFile to isolate its environment before invoking the daemon-discovery logic: set HOME to a temporary directory and call clearRelayfileEnv(t), matching the neighboring tests. Keep the existing PID state setup and assertions unchanged.cmd/relayfile-cli/main.go (3)
6502-6521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated delegated-to-legacy credential fallback discards the delegated error. Both call sites repeat the same four-step fallback and drop the delegated error when a saved token exists. Extract one helper that returns the resolved token and server, and log the discarded error inside it.
cmd/relayfile-cli/main.go#L6502-L6521: replace the two inline fallback branches with the shared helper, and keep theserverProvidedguard by passing it in.cmd/relayfile-cli/main.go#L7127-L7147: replace the two inline fallback branches with the same helper, and setdirectTokenandcredsFilefrom its result.🤖 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 6502 - 6521, Extract the duplicated delegated-to-legacy credential fallback into a shared helper that accepts the server-provided guard, logs the discarded delegated error, and returns the resolved token and server. In cmd/relayfile-cli/main.go lines 6502-6521, replace both inline fallback branches with this helper while preserving serverProvided behavior; in lines 7127-7147, use the same helper and assign its results to directToken and credsFile.
11503-11519: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueContinue scanning after an explicit boolean value.
The
=branch returns on the first matching field. For--background=false --background, the function returnsfalse, but Go'sflagpackage applies the last occurrence and yieldstrue. Track the value and return it after the loop instead.♻️ Proposed change
func commandHasEnabledBoolFlag(fields []string, name string) bool { longFlag := "--" + name shortFlag := "-" + name + enabled := false for _, field := range fields { if field == longFlag || field == shortFlag { - return true + enabled = true + continue } if strings.HasPrefix(field, longFlag+"=") || strings.HasPrefix(field, shortFlag+"=") { value := strings.TrimSpace(strings.TrimPrefix(strings.TrimPrefix(field, longFlag+"="), shortFlag+"=")) if value == "" { - return true + enabled = true + continue } parsed, err := strconv.ParseBool(value) - return err != nil || parsed + enabled = err != nil || parsed } } - return false + return enabled }🤖 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 11503 - 11519, Update commandHasEnabledBoolFlag to retain the latest matching boolean flag value instead of returning from the `=` branch immediately. Continue scanning all fields so later occurrences override earlier ones, matching Go flag behavior; preserve the existing handling for empty values and parse errors, then return the tracked result after the loop.
10672-10678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence the
nilerrfinding and document the soft failure.
catalogWorkspaceIDForRequestreturnsnilafterloadWorkspaceCatalogfails. golangci-lint reports this as anilerrerror, so the lint step fails. The behavior looks intentional: a missing or unreadable catalog must not block resolution, because callers fall back to the raw value. Make that intent explicit.♻️ Proposed change
func catalogWorkspaceIDForRequest(name, token string) (string, bool, error) { catalog, err := loadWorkspaceCatalog() if err != nil { - return "", false, nil + // A missing or unreadable catalog cannot canonicalize the request. + // Callers fall back to the requested value, so this is not fatal. + return "", false, nil //nolint:nilerr // soft-fail: no catalog means no canonical ID } return catalogWorkspaceIDFromCatalogForRequest(catalog, name, token) }🤖 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 10672 - 10678, Update catalogWorkspaceIDForRequest to document and explicitly preserve the intentional soft-failure behavior when loadWorkspaceCatalog returns an error: return no workspace ID without propagating the catalog-loading error, while keeping successful catalog resolution unchanged. Make the non-nil error handling explicit so golangci-lint no longer reports nilerr.Source: Linters/SAST tools
internal/mountsync/syncer.go (1)
6049-6086: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the conflict/preserve race on a ReadOnly path.
Both new branches require
canWriteto be true. Before overwriting divergent local content, the syncer compares local and remote hashes with the tracked base. It preserves local edits for duplicate or stale remote events whose content still matches the base, while concurrent divergence creates a conflict artifact using the tracked revision. WhencanWriteisfalse, neither branch runs, soshouldWritestaystrueand the remote content silently replaces local content, even if both sides diverged from the tracked base. No conflict artifact is written in that case.This is likely intentional, since a ReadOnly path should not have legitimate local edits. Confirm this is the desired behavior, and add a test that exercises the same race on a ReadOnly path (mirroring
TestAssessSameFileWebSocketBeatsDebouncedWatcher) to lock in the intended outcome.🤖 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 `@internal/mountsync/syncer.go` around lines 6049 - 6086, Confirm the intended ReadOnly behavior and add a regression test mirroring TestAssessSameFileWebSocketBeatsDebouncedWatcher for the same debounced-local-edit versus WebSocket race. Configure the path so canWrite is false and both local and remote content diverge from the tracked base, then assert the remote content replaces the local content, no conflict artifact is created, and the path remains consistent with ReadOnly semantics.
🤖 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 2710-2724: Move the background child cleanup in the test around
spawnBackgroundMountProcess to immediately after it returns, before validating
structured, Registered, and PID state. Guard cleanup with state.PID > 0 so
invalid pidfile data does not trigger an invalid process lookup, while ensuring
any registered child is stopped even when the subsequent validation calls
t.Fatalf.
---
Nitpick comments:
In `@cmd/relayfile-cli/main_test.go`:
- Around line 2269-2281: Update
TestRunningMountDaemonsExcludesCallerPIDFromPIDFile to isolate its environment
before invoking the daemon-discovery logic: set HOME to a temporary directory
and call clearRelayfileEnv(t), matching the neighboring tests. Keep the existing
PID state setup and assertions unchanged.
In `@cmd/relayfile-cli/main.go`:
- Around line 6502-6521: Extract the duplicated delegated-to-legacy credential
fallback into a shared helper that accepts the server-provided guard, logs the
discarded delegated error, and returns the resolved token and server. In
cmd/relayfile-cli/main.go lines 6502-6521, replace both inline fallback branches
with this helper while preserving serverProvided behavior; in lines 7127-7147,
use the same helper and assign its results to directToken and credsFile.
- Around line 11503-11519: Update commandHasEnabledBoolFlag to retain the latest
matching boolean flag value instead of returning from the `=` branch
immediately. Continue scanning all fields so later occurrences override earlier
ones, matching Go flag behavior; preserve the existing handling for empty values
and parse errors, then return the tracked result after the loop.
- Around line 10672-10678: Update catalogWorkspaceIDForRequest to document and
explicitly preserve the intentional soft-failure behavior when
loadWorkspaceCatalog returns an error: return no workspace ID without
propagating the catalog-loading error, while keeping successful catalog
resolution unchanged. Make the non-nil error handling explicit so golangci-lint
no longer reports nilerr.
In `@internal/mountsync/syncer.go`:
- Around line 6049-6086: Confirm the intended ReadOnly behavior and add a
regression test mirroring TestAssessSameFileWebSocketBeatsDebouncedWatcher for
the same debounced-local-edit versus WebSocket race. Configure the path so
canWrite is false and both local and remote content diverge from the tracked
base, then assert the remote content replaces the local content, no conflict
artifact is created, and the path remains consistent with ReadOnly semantics.
🪄 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: aa6e201e-45e8-4b83-933e-1be926daa1c4
📒 Files selected for processing (5)
cmd/relayfile-cli/main.gocmd/relayfile-cli/main_test.gointernal/mountsync/assessment_propagation_test.gointernal/mountsync/mount_root_clobber_test.gointernal/mountsync/syncer.go
| if err := spawnBackgroundMountProcess(args, []string{"/"}, localDir, pidFile, logFile, mountscope.LayoutExact); err != nil { | ||
| logBytes, _ := os.ReadFile(logFile) | ||
| t.Fatalf("spawnBackgroundMountProcess failed: %v\nlog:\n%s", err, logBytes) | ||
| } | ||
| state, structured := readDaemonPIDStateFile(pidFile) | ||
| if !structured || !state.Registered || state.PID <= 0 { | ||
| t.Fatalf("background child did not register: structured=%v state=%+v", structured, state) | ||
| } | ||
| t.Cleanup(func() { | ||
| if processAlive(state.PID) { | ||
| if process, err := os.FindProcess(state.PID); err == nil { | ||
| _ = forceDaemonStop(process) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect spawnBackgroundMountProcess for a registration wait.
set -euo pipefail
fd -t f 'main.go' cmd/relayfile-cli | while IFS= read -r file; do
ast-grep run --lang go \
--pattern 'func spawnBackgroundMountProcess($$$) $$$ { $$$ }' "$file" || true
done
rg -nP --type=go -C4 'spawnBackgroundMountProcessFn|func spawnBackgroundMountProcess|readDaemonPIDStateFile|waitForDaemonRegistration'Repository: AgentWorkforce/relayfile
Length of output: 4023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map and inspect the relevant functions around spawnBackgroundMountProcess and waitForBackgroundMountRegistration.
fd -t f 'main.go' cmd/relayfile-cli 2>/dev/null | while IFS= read -r file; do
ast-grep run --lang go --pattern 'func waitForBackgroundMountRegistration($$$) $$$ { $$$ }' "$file" || true
ast-grep run --lang go --pattern 'func readDaemonPIDStateFile($$$) $$$ { $$$ }' "$file" || true
done
printf '\n--- spawnBackgroundMountProcess and related wait code ---\n'
sed -n '12736,12805p' cmd/relayfile-cli/main.go
printf '\n--- waitForBackgroundMountRegistration implementation ---\n'
rg -n --type=go -C 12 'func waitForBackgroundMountRegistration' cmd/relayfile-cli/main.go
printf '\n--- readDaemonPIDStateFile implementation ---\n'
rg -n --type=go -C 8 'func readDaemonPIDStateFile' cmd/relayfile-cli/main.go
printf '\n--- processAlive implementation ---\n'
rg -n --type=go -C 6 'func processAlive|processAlive\(' cmd/relayfile-cli/main.go
printf '\n--- TestSpawnBackgroundMountProcessRegistersRealChild surrounding code ---\n'
sed -n '2674,2745p' cmd/relayfile-cli/main_test.goRepository: AgentWorkforce/relayfile
Length of output: 13030
Register the background-mount child cleanup before asserting on the pidfile.
spawnBackgroundMountProcess already waits for daemon state registration, so the immediate pidfile read is not racy. However, t.Cleanup is registered after the assertion at lines 2715-2717. If the assertion fails and calls t.Fatalf, the spawned child is never stopped. Register the cleanup with a guarded state.PID > 0 check before any t.Fatalf for pidstate validation.
🤖 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 2710 - 2724, Move the background
child cleanup in the test around spawnBackgroundMountProcess to immediately
after it returns, before validating structured, Registered, and PID state. Guard
cleanup with state.PID > 0 so invalid pidfile data does not trigger an invalid
process lookup, while ensuring any registered child is stopped even when the
subsequent validation calls t.Fatalf.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/mountsync/syncer.go (1)
6064-6101: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the remote revision for content-equivalent remote writes.
At
internal/mountsync/syncer.golines 6087-6093, the path preserves local bytes when remote content matches the tracked hash, but it does not updatetracked.Revision. A later writeback can send the oldbaseRevision, causing an existing conflict artifact and losing the remote progress. Move revision advancement into this branch and add the baserev_1, remoterev_2same-contents, divergent-local case.🤖 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 `@internal/mountsync/syncer.go` around lines 6064 - 6101, The content-equivalent remote-write branch in the tracked-entry apply logic must also persist the incoming remote revision. Update the branch guarded by !tracked.Dirty and !remoteDivergedFromBase to assign tracked.Revision from the remote revision before suppressing the write, while preserving the local bytes; add coverage for base rev_1, remote rev_2 with identical content, and divergent local content, verifying later writeback uses rev_2.
🤖 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 `@internal/mountsync/assessment_propagation_test.go`:
- Around line 562-564: Bound the eventResult wait in the test around
applyWebSocketEvent so it cannot block indefinitely. Replace the direct channel
receive with a local timeout mechanism that fails the test promptly if
completion does not arrive, while preserving the existing
duplicate-websocket-apply error assertion when eventResult returns.
---
Outside diff comments:
In `@internal/mountsync/syncer.go`:
- Around line 6064-6101: The content-equivalent remote-write branch in the
tracked-entry apply logic must also persist the incoming remote revision. Update
the branch guarded by !tracked.Dirty and !remoteDivergedFromBase to assign
tracked.Revision from the remote revision before suppressing the write, while
preserving the local bytes; add coverage for base rev_1, remote rev_2 with
identical content, and divergent local content, verifying later writeback uses
rev_2.
🪄 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: 9b99fa9f-9be7-40d5-8e2e-3914b1bf5bda
📒 Files selected for processing (3)
cmd/relayfile-cli/main_test.gointernal/mountsync/assessment_propagation_test.gointernal/mountsync/syncer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/relayfile-cli/main_test.go
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
1 participant
Summary
Three separate commits address the three reproduced mount defects from the 2026-08-03 resume brief.
Validation
Bug 1:
Bug 2:
Bug 3:
Full local gate: go test ./... -count=1 passed after all implementation changes. Complete race suites passed for internal/mountsync and cmd/relayfile-cli. All required GitHub checks, including E2E, are green.
Operational note: the Trail executable was unavailable both globally and in node_modules/.bin, so this run could not record or compact a trajectory. The existing .trajectories store remains present and intact for a later CLI-enabled run.
Independent review approved all three commits. Draft PR; do not merge.