Skip to content

Resolve file paths relative to repo root#127

Open
hassaanm wants to merge 1 commit into
jnsahaj:mainfrom
hassaanm:fix-subdirectory-file-paths
Open

Resolve file paths relative to repo root#127
hassaanm wants to merge 1 commit into
jnsahaj:mainfrom
hassaanm:fix-subdirectory-file-paths

Conversation

@hassaanm

@hassaanm hassaanm commented Jan 28, 2026

Copy link
Copy Markdown
Contributor

Summary

This pull request allows lumen to run correctly from a sub directory. Currently, it can't find the files correctly and treats them as being deleted.

Changes

  • Add get_repository_root() method to VcsBackend trait
  • Implement for GitBackend (using repo.workdir()) and JjBackend (using workspace_path)
  • Update get_new_content() to resolve paths from repo root

Test plan

  • Added test_get_repository_root_from_subdirectory for GitBackend
  • Added test_load_file_diffs_from_subdirectory integration test
  • Manually verified on an existing git repo but couldn't verify on a jj repo (see Notes section below)

Repro steps

  1. Go to a sub directory
  2. Make a change to a file
  3. Run lumen diff

Notes

Discovered a pre-existing issue: JjBackend cannot be loaded from subdirectories because Workspace::load() requires the exact workspace root path (unlike git2's Repository::discover() which walks up the directory tree). This is unrelated to this fix - the same error occurs on the current release.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected file diff/new-content resolution when running the tool from a subdirectory — diffs and updated content are now resolved against the repository root.
  • Tests

    • Added tests to verify diffs and file content are correctly loaded when operating from within repository subdirectories.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Jan 28, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds VcsBackend::get_repository_root and implements it for Git and Jj backends; diff logic now resolves working-tree file reads against the repository root with a fallback to the original filename. Tests added to verify root resolution from subdirectories.

Changes

Cohort / File(s) Summary
Vcs trait
src/vcs/backend.rs
Added fn get_repository_root(&self) -> Result<std::path::PathBuf, VcsError> to VcsBackend.
Git backend
src/vcs/git.rs
Added get_repository_root() for GitBackend (returns repo root or error for bare repo). Added unit tests verifying resolution from a subdirectory.
Jj backend
src/vcs/jj.rs
Implemented get_repository_root() for JjBackend (returns workspace_root).
Diff logic & tests
src/command/diff/git.rs
WorkingTree::get_new_content now resolves filenames against backend.get_repository_root() and falls back to the original path. Added test test_load_file_diffs_from_subdirectory.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as CLI/Command
  participant Diff as diff::git::WorkingTree
  participant VCS as VcsBackend
  participant FS as Filesystem

  CLI->>Diff: request file diff
  Diff->>VCS: get_repository_root()
  VCS-->>Diff: repository root path
  Diff->>FS: read file at (repo_root + filename)
  alt read succeeds
    FS-->>Diff: new_content
  else fallback
    Diff->>FS: read file at original filename
    FS-->>Diff: new_content or error
  end
  Diff-->>CLI: return diff result (includes new_content)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • jnsahaj

Poem

🐰 I hopped from subdir, nose to root,

I sniffed the path, then gave a toot.
Files now found where roots abide,
Diffs no longer lose their stride.
🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: resolving file paths relative to the repository root instead of the current working directory.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

@hassaanm hassaanm force-pushed the fix-subdirectory-file-paths branch from 49f03c5 to 8b63b7a Compare January 28, 2026 18:33

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/vcs/jj.rs`:
- Around line 973-975: get_repository_root currently returns the stored
self.workspace_path which may be a subdirectory; change get_repository_root to
return the actual workspace root by calling
self.workspace.workspace_root().to_path_buf() instead of using
self.workspace_path so the function returns the correct root loaded by the
Workspace (update the body of fn get_repository_root to return
Ok(self.workspace.workspace_root().to_path_buf())).

Comment thread src/vcs/jj.rs
When running lumen from a subdirectory, files appeared as "deleted"
because get_new_content() resolved paths relative to CWD instead of
the repository root.

- Add get_repository_root() method to VcsBackend trait
- Implement for GitBackend using repo.workdir()
- Implement for JjBackend using workspace_path
- Update get_new_content() to resolve paths from repo root
- Add tests for both the new method and the bug fix
@hassaanm hassaanm force-pushed the fix-subdirectory-file-paths branch from 8b63b7a to 94cdff3 Compare January 28, 2026 19:08
@Straffern

Copy link
Copy Markdown

This might be duplicate of: #110
My PR was implemented by llm, so if you know Rust, your solution might be better.

@hassaanm

hassaanm commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

Oh nice. I'm not too familiar with rust either. My pull request was ai assisted. I built and tested it locally for verification to help build confidence. I'll be happy if either pull request gets merged haha.

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.

2 participants