fix(mount): default LazyRepos to off (opt-in for huge-org workspaces)#121
fix(mount): default LazyRepos to off (opt-in for huge-org workspaces)#121khaliqgant merged 3 commits intomainfrom
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds lazy materialization of GitHub repositories as an optional feature controlled by the ChangesLazy Repository Materialization Configuration
Evals Runner — OpenRouter Human Review
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant MountMain
participant Syncer
participant FUSE
participant Remote
User->>CLI: invoke relayfile mount (--lazy-repos)
CLI->>MountMain: parsed flag (default from env)
MountMain->>Syncer: build SyncerOptions (LazyRepos pointer)
MountMain->>FUSE: build mountfuse.Config (LazyRepos)
Syncer->>Remote: sync/decide eager vs lazy (env/opts)
FUSE->>Remote: first ls/stat triggers materialize when lazy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
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: 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)
633-645:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
NewSyncercurrently overrides explicitSyncerOptions.LazyReposwith process envThis makes caller intent non-deterministic (for example, an explicit
--lazy-repos=falsecan still end up enabled if env is set), which breaks expected flag-over-env precedence.💡 Proposed fix
lazyRepos := opts.LazyRepos - if raw := strings.TrimSpace(os.Getenv("RELAYFILE_LAZY_REPOS")); raw != "" { - if parsed, perr := strconv.ParseBool(raw); perr == nil { - lazyRepos = parsed - } else if opts.Logger != nil { - opts.Logger.Printf("ignoring invalid RELAYFILE_LAZY_REPOS=%q: %v", raw, perr) - } - } else if raw := strings.TrimSpace(os.Getenv("RELAYFILE_MOUNT_LAZY_GITHUB_REPOS")); raw != "" { - if parsed, perr := strconv.ParseBool(raw); perr == nil { - lazyRepos = parsed - } else if opts.Logger != nil { - opts.Logger.Printf("ignoring invalid RELAYFILE_MOUNT_LAZY_GITHUB_REPOS=%q: %v", raw, perr) - } - }If env fallback is still needed for non-CLI callers, resolve it at the call site once (or move to a tri-state option) so precedence is explicit.
🤖 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 633 - 645, NewSyncer currently lets environment variables override an explicit SyncerOptions.LazyRepos; make precedence explicit by treating LazyRepos as an optional tri-state: change SyncerOptions.LazyRepos to *bool and in NewSyncer only consult RELAYFILE_LAZY_REPOS / RELAYFILE_MOUNT_LAZY_GITHUB_REPOS when opts.LazyRepos == nil, otherwise use the provided *opts.LazyRepos value to set local lazyRepos; keep the same parse+log behavior (opts.Logger.Printf) for invalid env values but do not mutate or override an explicitly set opts.LazyRepos.
🤖 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/mountfuse/fs.go`:
- Around line 168-170: newFSState currently re-checks environment via
lazyGithubReposEnabled() and can override an explicit cfg.LazyRepos; remove the
env re-read so the resolved config is authoritative: in newFSState only check
cfg.LazyRepos (i.e., if cfg.LazyRepos { state.lazyRepos =
NewLazyMaterializeCache() }) and delete the lazyGithubReposEnabled() branch so
state.lazyRepos is set solely based on cfg.LazyRepos.
---
Outside diff comments:
In `@internal/mountsync/syncer.go`:
- Around line 633-645: NewSyncer currently lets environment variables override
an explicit SyncerOptions.LazyRepos; make precedence explicit by treating
LazyRepos as an optional tri-state: change SyncerOptions.LazyRepos to *bool and
in NewSyncer only consult RELAYFILE_LAZY_REPOS /
RELAYFILE_MOUNT_LAZY_GITHUB_REPOS when opts.LazyRepos == nil, otherwise use the
provided *opts.LazyRepos value to set local lazyRepos; keep the same parse+log
behavior (opts.Logger.Printf) for invalid env values but do not mutate or
override an explicitly set opts.LazyRepos.
🪄 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: cce59ec5-2bd2-4540-a3cc-c03b96e489d3
📒 Files selected for processing (8)
README.mdcmd/relayfile-mount/fuse_mount.gocmd/relayfile-mount/main.gocmd/relayfile-mount/main_test.gointernal/mountfuse/fs.gointernal/mountfuse/layout.gointernal/mountsync/syncer.gointernal/mountsync/syncer_test.go
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| pathByInode: map[uint64]string{1: normalizeRemotePath(cfg.RemoteRoot)}, | ||
| } | ||
| if lazyGithubReposEnabled() { | ||
| if cfg.LazyRepos || lazyGithubReposEnabled() { |
There was a problem hiding this comment.
🟡 FUSE path: --lazy-repos=false cannot override env var due to || with lazyGithubReposEnabled()
In newFSState, the condition cfg.LazyRepos || lazyGithubReposEnabled() at internal/mountfuse/fs.go:168 means the env var RELAYFILE_LAZY_REPOS (or RELAYFILE_MOUNT_LAZY_GITHUB_REPOS) is read twice: once by the CLI to set the flag default (cmd/relayfile-mount/main.go:70), and again directly from the environment by lazyGithubReposEnabled(). When a user sets RELAYFILE_LAZY_REPOS=true and explicitly passes --lazy-repos=false, the CLI correctly resolves cfg.LazyRepos = false, but lazyGithubReposEnabled() re-reads the env var and returns true, so the || evaluates to true and lazy repos is enabled anyway. The explicit CLI flag is silently ignored on the FUSE path.
| if cfg.LazyRepos || lazyGithubReposEnabled() { | |
| if cfg.LazyRepos { |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if raw := strings.TrimSpace(os.Getenv("RELAYFILE_LAZY_REPOS")); raw != "" { | ||
| if parsed, perr := strconv.ParseBool(raw); perr == nil { | ||
| lazyRepos = parsed | ||
| } else if opts.Logger != nil { | ||
| opts.Logger.Printf("ignoring invalid RELAYFILE_LAZY_REPOS=%q: %v", raw, perr) | ||
| } | ||
| } else if raw := strings.TrimSpace(os.Getenv("RELAYFILE_MOUNT_LAZY_GITHUB_REPOS")); raw != "" { | ||
| if parsed, perr := strconv.ParseBool(raw); perr == nil { | ||
| lazyRepos = parsed | ||
| } else if opts.Logger != nil { |
There was a problem hiding this comment.
🟡 Syncer path: env var unconditionally overrides opts.LazyRepos, defeating --lazy-repos=false CLI flag
In NewSyncer at internal/mountsync/syncer.go:633-645, the env var RELAYFILE_LAZY_REPOS is read and unconditionally overrides opts.LazyRepos. When the CLI is the caller, opts.LazyRepos has already been resolved from the same env var plus the explicit --lazy-repos flag (cmd/relayfile-mount/main.go:70,112,167). If a user has RELAYFILE_LAZY_REPOS=true and passes --lazy-repos=false, the CLI sets opts.LazyRepos = false, but the syncer re-reads the env var and overrides it back to true. This is the same double-reading issue as the FUSE path but on the poll mode side.
Scenario trace
- CLI:
lazyReposEnv()readsRELAYFILE_LAZY_REPOS=true→ default =true - User passes
--lazy-repos=false→*lazyRepos = false cfg.lazyRepos = false→opts.LazyRepos = false- Syncer:
lazyRepos := opts.LazyRepos→false - Syncer:
os.Getenv("RELAYFILE_LAZY_REPOS")→"true"→lazyRepos = true← override!
(Refers to lines 633-645)
Prompt for agents
The env var override in NewSyncer at syncer.go:633-645 unconditionally re-reads RELAYFILE_LAZY_REPOS and RELAYFILE_MOUNT_LAZY_GITHUB_REPOS, overriding the opts.LazyRepos value that the CLI has already resolved from those same env vars plus the --lazy-repos flag. This defeats the explicit CLI flag.
The challenge is that NewSyncer is also called from non-CLI code (tests, programmatic use) where opts.LazyRepos might be the zero value and the env var should serve as a fallback. A bool field has no sentinel to distinguish 'not explicitly set' from 'explicitly set to false.'
Possible approaches:
1. Remove the env var re-reading from NewSyncer entirely and rely on callers to resolve env vars (simplest, but breaks non-CLI callers that rely on the env var fallback).
2. Change SyncerOptions.LazyRepos from bool to *bool so nil means 'not set' and allows the env var fallback, while an explicit false means 'explicitly disabled.'
3. Have the CLI mark that the env var was already resolved (e.g., a separate field like LazyReposExplicit bool), so the syncer only applies the env var override when not explicitly set.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Tests