Skip to content

Fix mount reconcile drift and local-dir footguns#174

Merged
khaliqgant merged 2 commits into
mainfrom
codex/issue-173-mount-sync-footguns
May 21, 2026
Merged

Fix mount reconcile drift and local-dir footguns#174
khaliqgant merged 2 commits into
mainfrom
codex/issue-173-mount-sync-footguns

Conversation

@khaliqgant
Copy link
Copy Markdown
Member

Summary

  • make forced full reconcile bypass quiet event-feed short-circuits and let quiet cycles reach the periodic full-pull safety net
  • make mount/start fall back to the recorded local mirror instead of CWD, with explicit --rehome required for intentional moves
  • guard --rehome against orphaning an already-running old daemon
  • treat path-like tree args for known mirror roots while preserving slash-containing workspace IDs

Fixes #173

Verification

  • go test ./cmd/relayfile-cli -run 'TestMountRequiresLocalDirWhenWorkspaceHasNoRecordedMirror|TestMountUsesRecordedLocalDirWhenOmitted|TestMountRefusesExplicitLocalDirThatRehomesRecordedMirror|TestMountRehomeRefusesRunningRecordedDaemon|TestMountRehomeAllowsExplicitMoveAndPersistsLocalDir|TestTreeTreatsPathLikeSingleArgAsRemotePath|TestTreePreservesUncatalogedSlashWorkspaceWhenRootIsUnknown|TestRestartRequiresRecordedLocalMirror' -count=3\n- go test ./internal/mountsync -run 'TestForceFullReconcileBypassesEventsShortCircuit|TestFullReconcileBypassesQuietEventsShortCircuit|TestQuietEventCyclesEventuallyRunPeriodicFullPull|TestPullShortCircuitsWhenNoNewEvents' -count=3\n- go test -count=1 ./...\n- scripts/check-contract-surface.sh\n- git diff --check\n\n## Review Notes\n- reviewed with subagents for mountsync correctness and CLI guardrails\n- patched follow-up findings around --rehome and slash-containing workspace IDs before opening this PR\n

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

Review Change Stack

Warning

Rate limit exceeded

@khaliqgant has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 42 minutes before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 43f1b207-8d1f-4950-9f81-d42e579926be

📥 Commits

Reviewing files that changed from the base of the PR and between 0670228 and aaf61c6.

📒 Files selected for processing (3)
  • .trajectories/index.json
  • cmd/relayfile-cli/main.go
  • cmd/relayfile-cli/main_test.go
📝 Walkthrough

Walkthrough

This PR addresses three functional issues from #173: (1) adds --rehome flag to prevent accidental re-homing of the local mirror; (2) fixes incremental syncer to trigger periodic full-tree pulls even on quiet event cycles so newer revisions are reliably pulled; (3) improves relayfile tree argument parsing to disambiguate remote paths from workspace names using known segments.

Changes

Mount re-home safety and LocalDir defaults

Layer / File(s) Summary
Mount flag definition and parsing
cmd/relayfile-cli/main.go
Introduces --rehome flag to mount subcommand, wires it into flag allowlist, and adjusts comment describing start as alias with --background detaching.
LocalDir resolution and re-home safety logic
cmd/relayfile-cli/main.go
Refactors LOCAL_DIR positional argument handling: introduces localDirExplicit tracking, defaults to recorded workspace LocalDir instead of current directory, refuses directory changes without --rehome, and checks daemon pid state (including stale pid cleanup) before allowing re-home. Updates record persistence to only save LocalDir when explicitly provided or when recorded state is blank.
Mount and start help text updates
cmd/relayfile-cli/main.go
Clarifies that relayfile start is an alias for mount (with --background required for daemon mode), documents the default LocalDir behavior, and adds help for new --rehome flag.
Mount CLI behavioral tests
cmd/relayfile-cli/main_test.go
Comprehensive test coverage for mount re-home safety: missing recorded mirror error, using recorded LocalDir when omitted, refusing explicit directory without --rehome, rejecting --rehome when daemon is running at old location, and successfully re-homing with --rehome while persisting updated LocalDir.

Full reconcile and quiet-cycle periodic pulls

Layer / File(s) Summary
Incremental events fast-path and periodic-pull logic
internal/mountsync/syncer.go
Updates pullRemote entry condition to check !s.forceFullReconcile alongside EventsCursor presence, rewrites "skip-if-no-events" short-circuit comment to document periodic-full-pull behavior and events-feed 404 fallback. Introduces local forceFullPull helper for consistent cadence reset and logging. Rewrites ListEvents result handling to optionally increment incrementalCycles on empty feeds, apply same threshold-check logic for both quiet and non-quiet cases, and return immediately unless periodic cadence triggers full pull.
Syncer reconciliation and quiet-cycle tests
internal/mountsync/syncer_test.go
Tests for full-reconcile bypass behavior: one asserts Reconcile with forceFullReconcile materializes newly-present files despite quiet-event condition; another tracks quiet-cycle cadence until periodic full pull triggers. Includes update to existing TestPullShortCircuitsWhenNoNewEvents to disable periodic full pulls (FullPullEvery=-1) for isolated quiet-cycle testing.
Bootstrap integration test for forced reconcile
internal/mountsync/bootstrap_test.go
Integration test seeding on-disk mount state with BootstrapComplete=true and stale EventsCursor, setting RELAYFILE_FORCE_FULL_RECONCILE=1 and disabling events feed, verifying that Reconcile executes full ListTree pull and materializes newly-added remote content despite quiet/short-circuit conditions.

Tree command path argument disambiguation

Layer / File(s) Summary
Tree argument parsing and path detection helpers
cmd/relayfile-cli/main.go
Modifies runTree to use new detection helper for identifying path-like arguments; when detected, normalizes path (adds leading /) and maintains workspace resolution. Adds helper functions: looksLikeRemotePathArg (checks against known remote-root segments and workspace IDs), normalizeCLIPathArg (prepends /), and knownRemoteRootSegment (recognizes provider-root mappings).
Tree CLI tests for path argument handling
cmd/relayfile-cli/main_test.go
Tests for path vs workspace argument disambiguation: one verifies path-like single argument is treated as remote path under env-selected workspace; another confirms workspace names containing slashes are URL-encoded while path defaults to / when root is unknown/uncataloged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • AgentWorkforce/relayfile#166: Both modify internal/mountsync/syncer.go reconciliation fast-path and full-pull triggering logic, particularly how forceFullReconcile affects short-circuiting and quiet-cycle behavior.
  • AgentWorkforce/relayfile#90: Both modify syncer incremental/events fast-path to trigger periodic/"trust but verify" full-tree pulls under quiet/no-events conditions with shared test coverage around full-pull cadence.
  • AgentWorkforce/relayfile#89: Both modify cmd/relayfile-cli/main.go to refine start alias behavior and mount/daemon startup semantics including lifecycle handling clarifications.

Poem

A rabbit bounds through mirrors bright,
No silent re-homing in the night—
With --rehome flags and periodic pulls,
Each newer revision the local syncer culls. 🐰✨
Tree paths now shine with clearer sight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix mount reconcile drift and local-dir footguns' clearly and concisely summarizes the main changes: addressing reconciliation issues and local directory safety concerns.
Description check ✅ Passed The description is comprehensive and related to the changeset, detailing the fixes for reconcile drift, local-dir fallback behavior, --rehome guardrails, and tree argument parsing.
Linked Issues check ✅ Passed The PR directly addresses all primary coding objectives from issue #173: functional sync bug fix (#1), local-dir re-homing guardrails (#3), mount/start CLI clarifications (#4), and tree path-parsing improvements (#6).
Out of Scope Changes check ✅ Passed All changes align with the stated objectives: syncer logic for full reconcile bypass, CLI mount/tree behavior changes with tests, and no unrelated modifications found.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/issue-173-mount-sync-footguns

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

Relayfile Eval Review

Run: .relayfile/evals/runs/2026-05-21T09-04-21-031Z-HEAD-provider
Mode: provider
Git SHA: a53e618

Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0

Human Review Cases

No reviewable human-review cases captured Relayfile output.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 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 `@cmd/relayfile-cli/main.go`:
- Around line 3754-3765: The code only looks up a workspace via
workspaceRecordByID(workspaceID) and so misses legacy records that have an empty
ID but the Name equals the workspace identifier; update the lookup logic so that
if workspaceRecordByID returns no record you also fall back to locating a legacy
record by name (e.g. call workspaceRecordByName(workspaceID) or scan stored
records for record.ID=="" && record.Name==workspaceID) and assign
recordedLocalDir from that record before using it to populate localDir; keep the
existing recordedLocalDir, workspaceRecordByID, and localDir variables unchanged
and only add the extra fallback branch to preserve recorded LocalDir for legacy
entries.
- Around line 3784-3795: The code currently removes a PID file whenever
verifyDaemonProcess returns verified==false, which wrongly treats “unproven
ownership” as a stale daemon; instead, change the rehome flow to refuse to
proceed (return an error) when pid != 0 && verified == false so we do not delete
mountPIDFile(recordedLocalDir) or orphan a potentially still-running daemon;
locate the block using verifyDaemonProcess(...) and mountPIDFile(...) and
replace the removal branch with a clear error return (or require an explicit
force flag) that instructs the user to stop the existing daemon or run with an
explicit override.
🪄 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: bc92712b-f40d-4a11-b626-db5fe9e54b8b

📥 Commits

Reviewing files that changed from the base of the PR and between e4f1c78 and 0670228.

📒 Files selected for processing (5)
  • cmd/relayfile-cli/main.go
  • cmd/relayfile-cli/main_test.go
  • internal/mountsync/bootstrap_test.go
  • internal/mountsync/syncer.go
  • internal/mountsync/syncer_test.go

Comment thread cmd/relayfile-cli/main.go
Comment thread cmd/relayfile-cli/main.go
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

Open in Devin Review

main.go: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_

**Preserve recorded `LocalDir` for legac
main.go: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_

**Don't treat every unverified pid file

Co-Authored-By: My Senior Dev <dev@myseniordev.com>
@khaliqgant khaliqgant merged commit d0f307d into main May 21, 2026
9 checks passed
@khaliqgant khaliqgant deleted the codex/issue-173-mount-sync-footguns branch May 21, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mount sync: local mirror doesn't pull newer remote revisions (+ mount/start CLI footguns)

1 participant