Skip to content

Path fetcher: Handle invalid paths - #317

Merged
edolstra merged 1 commit into
mainfrom
path-fetcher-invalid
Jan 16, 2026
Merged

edolstra merged 1 commit into
mainfrom
path-fetcher-invalid

Conversation

@edolstra

@edolstra edolstra commented Jan 15, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

Fixes a regression caused by #312 when using a chroot store and a path:... input that refers to the host store.

Context

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved system stability by enhancing validation checks in file path handling, ensuring cache entries are only created when necessary information is available.

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

This is especially important when using a chroot store and `path:...`
refers to the host store.
@coderabbitai

coderabbitai Bot commented Jan 15, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Path fetcher cache prefill logic updated to use conditional querying. Instead of unconditionally querying path info via store.queryPathInfo, the code now uses store.maybeQueryPathInfo and only populates fingerprint and cache entry when path info is available, skipping the operation if info is absent.

Changes

Cohort / File(s) Summary
Path Fetcher Cache Safety
src/libfetchers/path.cc
Changed cache prefill from unconditional queryPathInfo to conditional maybeQueryPathInfo, writing cache entry only when path info is present

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A safer path we now explore,
With queries gentle, not so brash,
Cache fills only when info's sure,
No empty writes in our stash! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Path fetcher: Handle invalid paths' directly aligns with the PR's objective of handling invalid paths in the path fetcher when using a chroot store, which is the primary change described in the summary and PR description.

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

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 322355f and 9a20bf5.

📒 Files selected for processing (1)
  • src/libfetchers/path.cc
🧰 Additional context used
🧬 Code graph analysis (1)
src/libfetchers/path.cc (1)
src/libfetchers/fetch-to-store.cc (2)
  • makeSourcePathToHashCacheKey (8-13)
  • makeSourcePathToHashCacheKey (9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build_x86_64-linux / build
  • GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (1)
src/libfetchers/path.cc (1)

159-165: LGTM! Graceful handling of invalid paths.

The change correctly addresses the regression by using maybeQueryPathInfo instead of queryPathInfo. When the path info is unavailable (e.g., host store path in a chroot store), the code now gracefully skips cache population rather than throwing, allowing fetchToStore() to proceed with the normal copy path.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

@github-actions

Copy link
Copy Markdown

@github-actions
github-actions Bot temporarily deployed to pull request January 15, 2026 13:59 Inactive
@edolstra
edolstra added this pull request to the merge queue Jan 15, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 15, 2026
@grahamc
grahamc added this pull request to the merge queue Jan 15, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 15, 2026
@edolstra
edolstra added this pull request to the merge queue Jan 16, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 16, 2026
@edolstra
edolstra added this pull request to the merge queue Jan 16, 2026
Merged via the queue into main with commit 3bea2c7 Jan 16, 2026
28 checks passed
@edolstra
edolstra deleted the path-fetcher-invalid branch January 16, 2026 17:24

This branch was previously deployed

1 inactive deployment
pull request — 9a20bf56 Deployed Jan 15, 2026 by github-actions[bot]
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