Search improvements#240
Conversation
WalkthroughAdds a literalSearch boolean option across the search flow. The flag is added to schemas, handler wiring, and SearchSessionOptions, and passed to ripgrep as -F for content searches. Server tool descriptions are updated to document usage. New tests validate literal vs. regex behavior. No other control flow changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant Handler as search-handlers
participant Manager as search-manager
participant RG as ripgrep
Client->>Server: StartSearch({ pattern, searchType, literalSearch? })
Server->>Handler: handleStartSearch(args)
Handler->>Manager: startSearch({ ...args, literalSearch })
alt searchType == "content"
opt literalSearch == true
note over Manager,RG: Build args with -F (fixed string)
end
Manager->>RG: rg [args]
RG-->>Manager: matches stream
else searchType == "files"
note over Manager: File search unchanged
Manager->>RG: rg [file args]
RG-->>Manager: paths stream
end
Manager-->>Handler: session/results
Handler-->>Server: response
Server-->>Client: results/status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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
🧹 Nitpick comments (9)
src/tools/schemas.ts (1)
118-119: Clarify/apply scope: literalSearch only affects content searchesToday it’s ignored in file-name searches. Consider a Zod refine to warn when literalSearch=true with searchType="files", or expand the comment to state it’s content-only.
Example refine (outside the object definition):
export const StartSearchArgsSchema = z.object({ // ... literalSearch: z.boolean().optional().default(false), }).refine( (d) => d.searchType !== 'files' || !d.literalSearch, { message: 'literalSearch applies only to searchType="content"', path: ['literalSearch'] } );src/search-manager.ts (2)
128-136: Add telemetry for literalSearch to aid analysisInclude the flag in the start event to understand usage patterns.
Apply near the existing capture:
capture('search_session_started', { sessionId, searchType: options.searchType, hasTimeout: !!timeoutMs, timeoutMs, requestedPath: options.rootPath, - validatedPath: validPath + validatedPath: validPath, + literalSearch: !!options.literalSearch });
309-387: Parity note: fallback search should respect literalSearchIf other paths (e.g., searchCodeFallback in src/tools/search.ts) are used on failures, they currently regex-match only. Consider threading literalSearch through and using fixed-string matching there too to avoid behavior drift.
src/handlers/search-handlers.ts (1)
39-46: Surface the matching mode in the user-facing headerHelps quickly confirm regex vs literal in logs/output.
let output = `Started ${searchTypeText} session: ${result.sessionId}\n`; output += `Pattern: "${parsed.data.pattern}"\n`; + if (parsed.data.searchType === 'content') { + output += `Mode: ${parsed.data.literalSearch ? 'literal' : 'regex'}\n`; + } output += `Path: ${parsed.data.path}\n`;src/server.ts (1)
339-354: Explicitly state scope of literalSearch in docsAdd a one-liner that the flag has no effect for file-name searches (glob mode).
PATTERN MATCHING MODES: - Default (literalSearch=false): Patterns are treated as regular expressions - Literal (literalSearch=true): Patterns are treated as exact strings + Note: literalSearch applies only to searchType="content". File-name searches always use glob patterns; the flag is ignored for searchType="files".test/test-literal-search.js (4)
33-38: Brittle sessionId extraction from formatted textPrefer structured data (e.g., include sessionId in metadata or as a separate text block with a stable prefix) to avoid regex fragility on message changes.
185-196: Outdated comment“This test should FAIL initially...” is stale after adding literalSearch. Update comment to reflect the intended passing state.
- * This test should FAIL initially since literalSearch parameter doesn't exist yet + * Verifies literalSearch finds exact matches for patterns with regex metacharacters
200-206: Misleading expectation in commentThe dataset contains one true occurrence; adjust the comment to avoid confusion.
- // Should find exactly 2 occurrences (one in each file: the exact match and in comment) + // Should find at least one exact occurrence
260-275: Assertion is very permissiveThe “hasResults” check treats both hits and “no matches” as pass. If the goal is exercising literal parsing, consider asserting at least one positive for a subset of patterns you know exist (e.g., array[0], obj.method(), count++).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/handlers/search-handlers.ts(1 hunks)src/search-manager.ts(2 hunks)src/server.ts(1 hunks)src/tools/schemas.ts(1 hunks)test/test-literal-search.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/search-manager.ts (1)
src/tools/search.ts (1)
searchTextInFiles(260-283)
src/tools/schemas.ts (1)
src/tools/edit.ts (2)
performSearchReplace(96-300)SearchReplace(12-15)
test/test-literal-search.js (2)
src/handlers/search-handlers.ts (3)
handleStartSearch(13-80)handleGetMoreSearchResults(85-170)handleStopSearch(175-210)src/config-manager.ts (1)
configManager(219-219)
src/handlers/search-handlers.ts (3)
test/test-search-code.js (1)
testInvalidArguments(437-467)src/tools/filesystem.ts (1)
searchFiles(905-970)src/tools/search.ts (1)
searchCodeFallback(149-257)
🔇 Additional comments (5)
src/tools/schemas.ts (1)
118-118: Schema addition looks goodliteralSearch with a safe default(false) is appropriate and non-breaking.
src/search-manager.ts (2)
40-41: Public option threading is correctAdding literalSearch?: boolean to SearchSessionOptions aligns with the new schema/handler.
316-320: Correct use of ripgrep -F gated to content modeNice, -F is only added for content searches, preserving file-search semantics.
src/handlers/search-handlers.ts (1)
34-35: Propagation wired correctlyliteralSearch is passed through to the search manager as expected.
test/test-literal-search.js (1)
8-10: Make tests resilient to a missing dist buildtest/test-literal-search.js (lines 8–10) imports from ../dist but the PR workspace has no dist directory (fd: "dist is not a directory"); either run the build step before running tests or change the imports to prefer ../dist when present and fall back to ../src (dynamic import / conditional require).
| async function teardown(originalConfig) { | ||
| console.log(`${colors.blue}Cleaning up literal search tests...${colors.reset}`); | ||
|
|
||
| // Clean up test directory | ||
| await fs.rm(LITERAL_SEARCH_TEST_DIR, { force: true, recursive: true }); | ||
|
|
||
| // Restore original config | ||
| await configManager.updateConfig(originalConfig); | ||
|
|
||
| console.log(`${colors.green}✓ Teardown complete: Test files removed and config restored${colors.reset}`); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure config restore API exists and is safe
Confirm configManager.updateConfig exists and round-trips the full config, or switch to restoring only changed keys to avoid unintended overrides.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'class\s+ConfigManager\b' -C3
rg -nP '\bupdateConfig\s*\(' -C2
rg -nP '\bsetValue\s*\(' -C2Length of output: 17716
Fix config restore: updateConfig merges instead of replacing
updateConfig(updates: Partial) does a merge (this.config = { ...this.config, ...updates }) so calling updateConfig(originalConfig) will NOT remove keys added after the snapshot — tests that rely on a full restore can leave config mutations behind.
Options: either make updateConfig replace the full config (assign/overwrite and persist) or update tests to restore only the changed keys (use configManager.setValue for each modified key) or add a dedicated resetConfig API.
Location: src/config-manager.ts (updateConfig implementation ~lines 201–203); tests call updateConfig(originalConfig) in many places (e.g. test/test-literal-search.js:162).
🤖 Prompt for AI Agents
In test/test-literal-search.js around lines 155–165, the teardown calls
configManager.updateConfig(originalConfig) but updateConfig merges instead of
replacing the config so keys added during tests are not removed; fix by
restoring only the mutated keys instead of calling updateConfig with the full
snapshot — capture the keys changed during the test (or compute diffs between
originalConfig and current config) and call configManager.setValue(key,
originalValue) for each key that was added/modified, or alternatively add and
use a new configManager.resetConfig(originalConfig) API that overwrites the
whole config and persists it; implement one of these two approaches and update
the teardown in this file to use it.
1 participant
Summary by CodeRabbit
New Features
Documentation