fix(git): honor the stored lfs_policy on materialize/hydrate (P6-GIT-04)#80
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements P6-GIT-04, enforcing the stored ChangesLFS Policy Enforcement on Materialize/Hydrate
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant materializeGitRepo
participant hydrateProjectUnlocked
participant applyMaterializeLFSPolicy
participant Runner
participant Store
materializeGitRepo->>hydrateProjectUnlocked: clone/hydrate local repo
hydrateProjectUnlocked-->>materializeGitRepo: success
materializeGitRepo->>applyMaterializeLFSPolicy: apply stored lfs_policy
applyMaterializeLFSPolicy->>applyMaterializeLFSPolicy: check usesLFS, normalize policy
alt policy is always/agent
applyMaterializeLFSPolicy->>Runner: LFSInstallLocal
applyMaterializeLFSPolicy->>Runner: LFSPull
Runner-->>applyMaterializeLFSPolicy: error or success
else policy is auto/never
applyMaterializeLFSPolicy->>applyMaterializeLFSPolicy: log warning (pointers remain)
end
applyMaterializeLFSPolicy-->>materializeGitRepo: return nil or error
alt error occurred
materializeGitRepo->>Store: UpdateProjectLocalState(failed, unknown)
materializeGitRepo-->>materializeGitRepo: return error, skip rebuild/env hydration
else success
materializeGitRepo->>materializeGitRepo: continue to dependency rebuild / env hydration
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The eager materialize/hydrate path never read project.LFSPolicy, so an lfs-policy=always repo materialized as silent pointer files recorded available/clean. Add Runner.LFSInstallLocal (install --local, required because gitEnv hides the global smudge filter) and applyMaterializeLFSPolicy, applied in materializeGitRepo (not inside hydrateProjectUnlocked, which the worktree path shares) so it covers the fresh clone and the SkeletonProjects retry of a repo recorded failed — never flipping it back to available with pointers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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 (3)
internal/cli/hydrate.go (1)
224-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared LFS policy dispatch
applyWorktreeLFSPolicyandapplyMaterializeLFSPolicystill duplicate the sameauto/nevervsalways/agentnormalization and branching; keep the path-specificLFSInstallLocalstep separate, but move the shared policy switch into one helper to avoid the two paths drifting again.🤖 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/cli/hydrate.go` around lines 224 - 254, `applyWorktreeLFSPolicy` and `applyMaterializeLFSPolicy` still duplicate the same LFSPolicy normalization and switch logic, so extract the shared `auto/never` vs `always/agent` dispatch into a helper and reuse it from both paths. Keep the path-specific `LFSInstallLocal` call only in `applyMaterializeLFSPolicy`, and have both callers delegate the common policy decision to the new helper so `dsgit.UsesLFS`, `LFSPull`, and the warning/default handling stay aligned.internal/cli/hydrate_lfs_test.go (1)
107-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate remote-seeding logic with
worktree_test.go.
createHydrateRemotere-implements the same bare-repo/seed/push sequence already present insetupFreshWorktreeRepo(internal/cli/worktree_test.go, lines 174-211), differing only in whether LFS-tracked content is committed. Consider extracting a shared helper (e.g.,newTestGitRemote(t, opts...)) parameterized by whether to add tracked LFS content, to avoid drift between the two copies as git setup evolves.🤖 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/cli/hydrate_lfs_test.go` around lines 107 - 136, `createHydrateRemote` duplicates the same bare-repo/seed/push setup already used by `setupFreshWorktreeRepo`, so refactor the shared git-remote bootstrapping into a common helper and keep only the LFS-specific file setup in `createHydrateRemote`. Extract the repeated remote initialization logic into a helper such as a test git remote builder that both `createHydrateRemote` and `setupFreshWorktreeRepo` can call, with a parameter or option to add LFS-tracked content when needed. Ensure the helper owns the shared init/config/commit/push flow so the two test paths cannot drift.internal/cli/worktree_test.go (1)
238-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCentralize this test-only mutation
Raw SQL here still couples the test to
git_repos/namespace_entriesschema details and SQLite DSN plumbing. A small test-only setter onstate.Storewould keep that knowledge in one place.🤖 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/cli/worktree_test.go` around lines 238 - 265, The test helper setProjectLFSPolicy currently reaches into the database with raw SQL and SQLite connection setup, which duplicates schema and DSN details inside the test. Move this mutation into a small test-only setter on state.Store and have setProjectLFSPolicy call that store method instead, so the knowledge of git_repos, namespace_entries, and the SQLite plumbing stays centralized.
🤖 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/cli/hydrate.go`:
- Around line 248-249: The warning in the hydrate path treats both auto and
never the same, but never is an intentional opt-out and should not suggest
switching to always. Update the policy handling in hydrate.go around the switch
on policy so the code that logs from logging.Logger(ctx).Warn distinguishes auto
from never, keeping the fetch-objects recommendation only for auto and either
suppressing or changing the message for never to reflect the explicit choice.
Use the existing policy variable and the materialize/sync warning path to locate
the change.
---
Nitpick comments:
In `@internal/cli/hydrate_lfs_test.go`:
- Around line 107-136: `createHydrateRemote` duplicates the same
bare-repo/seed/push setup already used by `setupFreshWorktreeRepo`, so refactor
the shared git-remote bootstrapping into a common helper and keep only the
LFS-specific file setup in `createHydrateRemote`. Extract the repeated remote
initialization logic into a helper such as a test git remote builder that both
`createHydrateRemote` and `setupFreshWorktreeRepo` can call, with a parameter or
option to add LFS-tracked content when needed. Ensure the helper owns the shared
init/config/commit/push flow so the two test paths cannot drift.
In `@internal/cli/hydrate.go`:
- Around line 224-254: `applyWorktreeLFSPolicy` and `applyMaterializeLFSPolicy`
still duplicate the same LFSPolicy normalization and switch logic, so extract
the shared `auto/never` vs `always/agent` dispatch into a helper and reuse it
from both paths. Keep the path-specific `LFSInstallLocal` call only in
`applyMaterializeLFSPolicy`, and have both callers delegate the common policy
decision to the new helper so `dsgit.UsesLFS`, `LFSPull`, and the
warning/default handling stay aligned.
In `@internal/cli/worktree_test.go`:
- Around line 238-265: The test helper setProjectLFSPolicy currently reaches
into the database with raw SQL and SQLite connection setup, which duplicates
schema and DSN details inside the test. Move this mutation into a small
test-only setter on state.Store and have setProjectLFSPolicy call that store
method instead, so the knowledge of git_repos, namespace_entries, and the SQLite
plumbing stays centralized.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 365118f3-a34f-4f64-90ee-ee0c5755c52d
📒 Files selected for processing (9)
docs/audits/README.mdinternal/cli/hydrate.gointernal/cli/hydrate_lfs_test.gointernal/cli/materialize.gointernal/cli/worktree_test.gointernal/git/git.gospec/08_GIT_MATERIALIZATION_AND_WORKTREES.mdspec/13_CLI_DAEMON_API.mdspec/18_WORK_LOG.md
Comment on lines
+248
to
+249
| case "auto", "never": | ||
| logging.Logger(ctx).Warn("git LFS pointer files remain; set lfs_policy=always to fetch objects", "path", project.Path, "lfs_policy", policy) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
never policy gets a misleading "switch to always" warning.
never is an explicit opt-out, not an oversight like auto, yet it gets the same "set lfs_policy=always to fetch objects" warning on every materialize/sync run. This creates persistent, actionable-sounding log noise for a deliberate configuration choice.
💡 Proposed fix to differentiate `never` from `auto`
case "auto", "never":
- logging.Logger(ctx).Warn("git LFS pointer files remain; set lfs_policy=always to fetch objects", "path", project.Path, "lfs_policy", policy)
+ if policy == "auto" {
+ logging.Logger(ctx).Warn("git LFS pointer files remain; set lfs_policy=always to fetch objects", "path", project.Path, "lfs_policy", policy)
+ }📝 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
| case "auto", "never": | |
| logging.Logger(ctx).Warn("git LFS pointer files remain; set lfs_policy=always to fetch objects", "path", project.Path, "lfs_policy", policy) | |
| case "auto", "never": | |
| if policy == "auto" { | |
| logging.Logger(ctx).Warn("git LFS pointer files remain; set lfs_policy=always to fetch objects", "path", project.Path, "lfs_policy", policy) | |
| } |
🤖 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/cli/hydrate.go` around lines 248 - 249, The warning in the hydrate
path treats both auto and never the same, but never is an intentional opt-out
and should not suggest switching to always. Update the policy handling in
hydrate.go around the switch on policy so the code that logs from
logging.Logger(ctx).Warn distinguishes auto from never, keeping the
fetch-objects recommendation only for auto and either suppressing or changing
the message for never to reflect the explicit choice. Use the existing policy
variable and the materialize/sync warning path to locate the change.
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.
P6-GIT-04 — honor the stored
lfs_policyon the eager materialize/hydrate pathOnly the worktree path honored
lfs_policy;hydrateProjectUnlocked(the primarydevstrap sync/materializeclone) never readproject.LFSPolicy. AndgitEnvsetsGIT_CONFIG_GLOBAL=/dev/null, hiding the user's globalgit lfs installsmudge filter — so anlfs-policy=alwaysrepo materialized as 3-line pointer files, which match the index →DirtyStateclean → recorded available/clean with zero warning, breaking "after sync the tree is really present."Change
internal/git/git.go—Runner.LFSInstallLocal(git lfs install --local); required because the global filter is hidden.internal/cli/hydrate.go—applyMaterializeLFSPolicymirrorsapplyWorktreeLFSPolicy:always/agent→ install +LFSPull(fail the project on error);auto/never→ warn.LFSPullcarries the P6-GIT-01 long-transfer timeout.internal/cli/materialize.go— applied inmaterializeGitRepo(the caller), not insidehydrateProjectUnlocked.Why the caller, not
hydrateProjectUnlocked(review-caught blocking bug)hydrateProjectUnlockedis shared withcreateFreshWorktree. Placing LFS inside it (a) fired on the worktree flow and (b) missed the retry: a repo recorded"failed"for an LFS pull failure is re-queued viaSkeletonProjects(which includes"failed") on the next sync/run-loop tick, hits the already-on-disk early-return, and silently flipped back to"available"/"clean"with pointers. Applying it inmaterializeGitRepocovers the fresh clone and the retry, and leaves the worktree path untouched.Tests
TestMaterializeLFSAutoRecordsAvailableWithPointers,…AlwaysRecordsFailedWhenPullFails,…AlwaysDoesNotFlipFailedToAvailableOnRetry(the retry invariant),…NoLFSUnaffected; the pre-existing worktree-cleanup test stays green.Validation
gofmtclean ·spec-driftpasses · fullgo test ./...green. Independent opus review (one blocking finding, fixed) + Codex implementation.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes