Conversation
- ripgrep_command: treat exit code 2 (permission denied) as partial error; return found files from stdout instead of discarding them - collector: continue with partial results when only one of vibing/md searches fails; show 'Partial error' warning instead of failing silently - fd_command: detect fdfind executable (Ubuntu/Debian apt package name) in addition to fd, so auto strategy picks up fd on Ubuntu
📝 WalkthroughWalkthroughThis change enhances fault tolerance across the file discovery pipeline by enabling partial result handling. The collector now retains results when one search succeeds despite the other failing. The fd_command detects the fd executable at runtime and adds pattern-based matching with mtime filtering. The ripgrep_command treats permission errors as non-fatal, returning partial results with warnings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
コードレビューこのPRは2つの実際のユーザー体験上の問題を修正しており、全体的な方向性は正しいと思います。以下に詳細なフィードバックをまとめます。 ✅ 良い点
🔍 気になる点・改善提案1.
|
![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)
lua/vibing/infrastructure/file_finder/ripgrep_command.lua (2)
30-32: Stale@returnannotation — partial_error is returned on partial success, not only on failure.The comment "only on failure" no longer holds: when exit code 2 occurs and files were found, the function returns both a non-empty
filestable and apartial_error. Callers relying on this doc annotation (e.g., generated LuaLS type-checks) will have an incorrect expectation.📝 Proposed annotation update
---@return string[] files Array of absolute paths to matched files ----@return string? error Error message (only on failure) +---@return string? error Error message on failure, or partial warning on exit code 2 (permission denied)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/infrastructure/file_finder/ripgrep_command.lua` around lines 30 - 32, Update the `@return` annotation for RipgrepCommand:find to reflect that it may return a non-empty files array along with a partial_error string on partial success (e.g., when exit code 2 occurs), not just on failure; locate the RipgrepCommand:find function and change the doc comment for the second return to indicate the error can be returned on partial success or failure (e.g., "string? partial_error Error message (returned on failure or partial success)").
63-69: "rg command failed:" prefix is misleading for a partial/warning result.Line 60 and line 68 use the identical
"rg command failed: "prefix, but they represent different severity levels — a complete failure vs. a non-fatal partial permission error. A caller that logs the raw message (e.g., for debugging) cannot distinguish the two. Consider a distinct prefix such as"rg partial error: ".📝 Suggested prefix change
- partial_error = "rg command failed: " .. first_error + partial_error = "rg partial error (permission denied): " .. first_error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/infrastructure/file_finder/ripgrep_command.lua` around lines 63 - 69, The current message for partial/non-fatal rg failures uses the same "rg command failed:" prefix as fatal failures; update the partial-error construction in the ripgrep_command.lua block where partial_error is set (when result.code == 2 and result.stderr) to use a distinct prefix such as "rg partial error:" (or similar) so callers can distinguish non-fatal permission/partial results from full failures; keep extracting the first stderr line via result.stderr:match("([^\n]+)") and only change the prefix text in the string concatenation that assigns partial_error.lua/vibing/infrastructure/file_finder/fd_command.lua (1)
10-20: Module-level cache would be beneficial fordetect_fd_executable()when custom options are used.The factory already caches finder instances for default calls, but when
get_finder()is called with custom options likemtime_daysorstrategy(as incollector.lua:219), the factory cache is bypassed. This meansFdCommand:new()runs again per call, triggering 1-2 blockingvim.system():wait()calls even though the executable name is invariant for a Neovim session. Similarly,get_available_strategies()creates multiple instances to check support.A module-level cache would reduce this to at most one detection per session, following the same pattern already used in the factory.
♻️ Proposed module-level cache
+---@type string? +local _cached_executable = nil +local _executable_detected = false + ---Detect fd executable name (fd or fdfind on Ubuntu/Debian) ---@return string? executable name or nil if not found local function detect_fd_executable() + if _executable_detected then + return _cached_executable + end for _, name in ipairs({ "fd", "fdfind" }) do local result = vim.system({ "which", name }, { text = true }):wait() if result.code == 0 then + _cached_executable = name + _executable_detected = true return name end end + _executable_detected = true return nil end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/vibing/infrastructure/file_finder/fd_command.lua` around lines 10 - 20, detect_fd_executable() runs blocking vim.system():wait() on every instantiation (e.g., FdCommand:new(), get_finder() with custom options, and get_available_strategies()), causing repeated work; add a module-level cache (e.g., local fd_executable_cache) and have detect_fd_executable return the cached value if set, otherwise perform the which check once and store the result; update any callers (FdCommand:new, get_finder, get_available_strategies) to rely on detect_fd_executable (no other changes) so that only a single detection happens per Neovim session.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/vibing/application/daily_summary/collector.lua`:
- Around line 22-39: The current logic misses emitting a warning when exactly
one search errors but neither returns results; change the partial-warning branch
to notify whenever any error exists (partial_err) instead of requiring
has_results, since the earlier block already handles the case where both
searches failed with no results (err1 and err2 and not has_results) and returns.
Update the condition around the vim.notify call that currently checks
partial_err and has_results to simply check partial_err (ref: err1, err2,
has_results, partial_err in collector.lua) so any single-search error produces a
diagnostic.
---
Nitpick comments:
In `@lua/vibing/infrastructure/file_finder/fd_command.lua`:
- Around line 10-20: detect_fd_executable() runs blocking vim.system():wait() on
every instantiation (e.g., FdCommand:new(), get_finder() with custom options,
and get_available_strategies()), causing repeated work; add a module-level cache
(e.g., local fd_executable_cache) and have detect_fd_executable return the
cached value if set, otherwise perform the which check once and store the
result; update any callers (FdCommand:new, get_finder, get_available_strategies)
to rely on detect_fd_executable (no other changes) so that only a single
detection happens per Neovim session.
In `@lua/vibing/infrastructure/file_finder/ripgrep_command.lua`:
- Around line 30-32: Update the `@return` annotation for RipgrepCommand:find to
reflect that it may return a non-empty files array along with a partial_error
string on partial success (e.g., when exit code 2 occurs), not just on failure;
locate the RipgrepCommand:find function and change the doc comment for the
second return to indicate the error can be returned on partial success or
failure (e.g., "string? partial_error Error message (returned on failure or
partial success)").
- Around line 63-69: The current message for partial/non-fatal rg failures uses
the same "rg command failed:" prefix as fatal failures; update the partial-error
construction in the ripgrep_command.lua block where partial_error is set (when
result.code == 2 and result.stderr) to use a distinct prefix such as "rg partial
error:" (or similar) so callers can distinguish non-fatal permission/partial
results from full failures; keep extracting the first stderr line via
result.stderr:match("([^\n]+)") and only change the prefix text in the string
concatenation that assigns partial_error.
| -- Warn about errors but continue with partial results | ||
| -- If both searches completely failed with no results, return empty | ||
| local has_results = (vibing_files and #vibing_files > 0) or (md_files and #md_files > 0) | ||
| if err1 and err2 and not has_results then | ||
| vim.notify( | ||
| string.format("vibing.nvim: Failed to search directory %s: %s", directory, err1), | ||
| vim.log.levels.WARN | ||
| ) | ||
| return {} | ||
| end | ||
| -- Warn about partial errors (permission denied on some subdirectories) | ||
| local partial_err = err1 or err2 | ||
| if partial_err and has_results then | ||
| vim.notify( | ||
| string.format("vibing.nvim: Partial error searching directory %s: %s", directory, partial_err), | ||
| vim.log.levels.WARN | ||
| ) | ||
| end |
There was a problem hiding this comment.
Silent failure when exactly one search errors and neither returns results.
The current condition if partial_err and has_results means a warning is only emitted when there are results plus a partial error. When one search completely fails (e.g., err1 = "rg command failed: …") and the other succeeds with zero results (err2 = nil, md_files = {}), all three conditions evaluate as:
| variable | value |
|---|---|
has_results |
false |
err1 and err2 and not has_results |
false (err2 is nil) → no early abort |
partial_err and has_results |
false → no warning |
The function returns {} silently. The user sees "no files found" with no diagnostic, masking a real error in one of the two finders.
🐛 Proposed fix — always warn when any error is present
- -- Warn about partial errors (permission denied on some subdirectories)
- local partial_err = err1 or err2
- if partial_err and has_results then
- vim.notify(
- string.format("vibing.nvim: Partial error searching directory %s: %s", directory, partial_err),
- vim.log.levels.WARN
- )
- end
+ -- Warn about partial errors (permission denied on some subdirectories)
+ -- or when one search failed outright while the other returned nothing
+ local partial_err = err1 or err2
+ if partial_err then
+ vim.notify(
+ string.format("vibing.nvim: Partial error searching directory %s: %s", directory, partial_err),
+ vim.log.levels.WARN
+ )
+ endNote: This makes the early-abort block above (which already notifies and returns
{}) the only path that suppresses the partial warning — specifically when both searches fail with no results. All other error states will emit a diagnostic.
📝 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.
| -- Warn about errors but continue with partial results | |
| -- If both searches completely failed with no results, return empty | |
| local has_results = (vibing_files and #vibing_files > 0) or (md_files and #md_files > 0) | |
| if err1 and err2 and not has_results then | |
| vim.notify( | |
| string.format("vibing.nvim: Failed to search directory %s: %s", directory, err1), | |
| vim.log.levels.WARN | |
| ) | |
| return {} | |
| end | |
| -- Warn about partial errors (permission denied on some subdirectories) | |
| local partial_err = err1 or err2 | |
| if partial_err and has_results then | |
| vim.notify( | |
| string.format("vibing.nvim: Partial error searching directory %s: %s", directory, partial_err), | |
| vim.log.levels.WARN | |
| ) | |
| end | |
| -- Warn about errors but continue with partial results | |
| -- If both searches completely failed with no results, return empty | |
| local has_results = (vibing_files and `#vibing_files` > 0) or (md_files and `#md_files` > 0) | |
| if err1 and err2 and not has_results then | |
| vim.notify( | |
| string.format("vibing.nvim: Failed to search directory %s: %s", directory, err1), | |
| vim.log.levels.WARN | |
| ) | |
| return {} | |
| end | |
| -- Warn about partial errors (permission denied on some subdirectories) | |
| -- or when one search failed outright while the other returned nothing | |
| local partial_err = err1 or err2 | |
| if partial_err then | |
| vim.notify( | |
| string.format("vibing.nvim: Partial error searching directory %s: %s", directory, partial_err), | |
| vim.log.levels.WARN | |
| ) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/vibing/application/daily_summary/collector.lua` around lines 22 - 39, The
current logic misses emitting a warning when exactly one search errors but
neither returns results; change the partial-warning branch to notify whenever
any error exists (partial_err) instead of requiring has_results, since the
earlier block already handles the case where both searches failed with no
results (err1 and err2 and not has_results) and returns. Update the condition
around the vim.notify call that currently checks partial_err and has_results to
simply check partial_err (ref: err1, err2, has_results, partial_err in
collector.lua) so any single-search error produces a diagnostic.
shabaraba
deleted the
fix/daily-summary-permission-denied-and-fd-detection
branch
1 participant
概要
VibingDailySummaryAllでアクセス権限のないディレクトリが含まれる場合にエラーで止まる問題と、Ubuntu でfdコマンドが検出されない問題を修正します。修正内容
1. Permission denied エラーでも処理を継続 (
ripgrep_command.lua,collector.lua)問題:
rgがアクセス権限のないディレクトリに遭遇すると exit code2を返します。以前の実装ではcode != 0 && code != 1の場合にエラーとして扱い、stdout に出力された部分的な結果も全て破棄していました。修正:
ripgrep_command: exit code2の場合はstdoutの内容（取得できたファイル）を返し、エラーはpartial_errorとして呼び出し元に伝えるcollector: 部分的なエラーがある場合は"Partial error searching directory"の警告を出しつつ取得済みのファイルで処理を継続。両方の検索が完全に失敗した場合のみ処理を中断する2. Ubuntu/Debian での
fdfind検出 (fd_command.lua)問題:
Ubuntu では
apt install fd-findでインストールされるfdコマンドの実行ファイル名がfdfindになります。which fdのみで判定していたため、auto 選択でfdが検出されずfindにフォールバックしていました。修正:
detect_fd_executable()関数でfdとfdfindを順番に確認し、見つかった方を使用するように変更。Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements