File improve search error reporting #249
Conversation
…locking user access - Replace path blocking with precise error filtering for analytics - Add shouldCaptureSearchError() method with specific patterns from production data - Filter out permission denied, timeouts, and system directory errors from analytics - Keep all search functionality intact - users can search anywhere they have permissions - Only filter noise from error reporting, not actual search capability Targeted patterns filtered: - Permission denied / Operation not permitted (multi-language) - Operation timed out / os error 60/4 - Photos Library.photoslibrary, Windows Defender, System directories - Interrupt/Interrupted system calls Real errors will still be captured for debugging.
# Conflicts: # src/search-manager.ts
WalkthroughAdds a wasIncomplete flag to search sessions and propagates it through startSearch/readSearchResults. The handler now appends a warning when a completed search reports wasIncomplete (e.g., due to permission-denied files). Telemetry cleanup references removed, and an exported stopSearchManagerCleanup() helper is deleted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Handler as search-handlers.ts
participant Manager as search-manager.ts
participant RG as ripgrep
Client->>Handler: Request search / get more results
Handler->>Manager: startSearch / readSearchResults
Manager->>RG: Execute search
RG-->>Manager: Streamed matches<br/>Exit code (0/2/...)
alt Exit code 2
Note over Manager: Set session.wasIncomplete = true
end
Manager-->>Handler: Results { isComplete, wasIncomplete?, ... }
alt isComplete && wasIncomplete
Handler->>Handler: Append warning about inaccessible files
end
Handler-->>Client: Final payload (with optional warning)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![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 (2)
src/search-manager.ts (2)
56-63: startSearch return type is missing wasIncomplete.Public shape claims to expose wasIncomplete, but it’s not in the Promise<…> return type here.
Apply this diff to include wasIncomplete in the return type:
async startSearch(options: SearchSessionOptions): Promise<{ sessionId: string; isComplete: boolean; isError: boolean; results: SearchResult[]; totalResults: number; runtime: number; + wasIncomplete?: boolean; }> {
150-157: startSearch return object doesn’t include wasIncomplete.If the process completes very quickly (including with exit code 2), callers won’t see the flag from startSearch.
Apply this diff to return the flag:
return { sessionId, isComplete: session.isComplete, isError: session.isError, results: [...session.results], totalResults: session.totalMatches, - runtime: Date.now() - session.startTime + runtime: Date.now() - session.startTime, + wasIncomplete: session.wasIncomplete };
🧹 Nitpick comments (3)
src/search-manager.ts (2)
27-28: Good addition: track incomplete searches.Adding wasIncomplete is useful for surfacing partial results due to access issues.
Consider making wasIncomplete non-optional and defaulting it to false in the session object to simplify downstream checks.
403-406: Avoid duplicating stderr into session.error.You first append raw stderr, then later append filtered “meaningful errors,” causing duplication and noisy user messages.
Apply this diff to only accumulate filtered errors:
- // Store error text for potential user display, but don't capture individual errors - // We'll capture incomplete search status in the completion event instead - session.error = (session.error || '') + errorText; + // Buffer stderr locally; only add filtered, meaningful errors to session.error below + const rawErrorText = errorText;Optionally keep rawErrorText in a private local scope; if you need it later, store it on a private field (not exposed to users).
src/handlers/search-handlers.ts (1)
62-66: Also warn on incompleteness in handleStartSearch when completed immediately.If a search completes before the first response, users won’t see the warning here unless you check result.wasIncomplete.
Apply this diff after writing the completion status line:
if (result.isComplete) { - output += `\n✅ Search completed.`; + output += `\n✅ Search completed.`; + if (result.wasIncomplete) { + output += `\n⚠️ Warning: Some files were inaccessible due to permissions. Results may be incomplete.`; + } } else {Note: Requires startSearch to return wasIncomplete (see search-manager.ts comments).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/search-handlers.ts(1 hunks)src/search-manager.ts(12 hunks)
🔇 Additional comments (6)
src/search-manager.ts (5)
178-179: LGTM: propagate wasIncomplete in readSearchResults.Including the flag in the tail/range response is consistent with the goal.
202-204: LGTM: tail response includes wasIncomplete.Propagation here looks correct.
222-224: LGTM: range response includes wasIncomplete.Consistent with the tail path.
471-473: LGTM: telemetry includes wasIncomplete.Good to surface this in completion event.
601-601: No action needed.Formatting-only change.
src/handlers/search-handlers.ts (1)
159-161: Nice UX: warning on incomplete searches.Clear, actionable message without blocking result delivery.
| // Track if search was incomplete due to access issues | ||
| // Ripgrep exit code 2 means "some files couldn't be searched" | ||
| if (code === 2) { | ||
| session.wasIncomplete = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Confirm ripgrep exit code semantics before relying on code === 2.
rg’s exit code 2 is generally “error,” not guaranteed “some files couldn’t be searched.” Permission-denied warnings often don’t change the exit code. Relying solely on code === 2 may miss/inflate wasIncomplete.
Proposed verification via web search:
As a fallback, consider setting wasIncomplete if stderr contains common permission patterns (e.g., "Permission denied", "Operation not permitted", "os error 13"), in addition to exit code checks.
🌐 Web query:
What are ripgrep (rg) exit codes and does exit code 2 specifically indicate permission-denied or just any error? Also, do permission-denied warnings affect exit status when matches are found?
💡 Result:
Short answers:
-
Exit code meanings: 0 = at least one match and no error; 1 = no match and no error; 2 = an error occurred. [1][2]
-
Exit code 2 is not only "permission denied" — it is emitted for any error (both catastrophic errors like bad regex and soft errors like being unable to read a file, e.g., permission denied). [1][2]
-
Permission-denied warnings count as errors and will normally make rg exit with status 2 even if matches are printed, except for the special case when you run with -q/--quiet and a match is found (then rg may exit 0). [1][2]
Sources: ripgrep man page and ripgrep changelog/release notes. [1][2]
Refine wasIncomplete logic
Ripgrep exit code 2 is a generic “error” (bad regex, I/O failures, etc.), not specifically “some files couldn’t be searched.” Only set session.wasIncomplete when stderr indicates access issues (e.g. /permission denied/i).
🤖 Prompt for AI Agents
In src/search-manager.ts around lines 443 to 448, the current logic marks
session.wasIncomplete whenever ripgrep exits with code === 2, but exit code 2 is
generic; update the logic to set session.wasIncomplete only when stderr contains
access-related messages (e.g. permission denied). Concretely: check that stderr
is a string (or coerce to one), test it with a case-insensitive regex like
/permission denied/i (and optionally other access phrases such as /access
denied|cannot read/i), and only set session.wasIncomplete = true when code === 2
AND the stderr regex matches; otherwise leave session.wasIncomplete false and
preserve existing error handling. Ensure no exceptions occur if stderr is
null/undefined by defaulting to an empty string before testing.
Summary by CodeRabbit
Improvements
Bug Fixes