refactor(web): unify tool definitions between ask agent and MCP server#1014
refactor(web): unify tool definitions between ask agent and MCP server#1014brendan-kellam wants to merge 21 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralizes and adds tool definitions and adapters, introduces a tools registry, updates MCP server wiring to register external tool definitions, refactors the chat agent and UI to consume structured tool metadata (output.metadata.*), and adds related UI components, types, and config/dependency changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Chat Agent
participant Tools as Tools Registry
participant ToolDef as Tool Definition
participant MCP as MCP Server
participant UI as Chat UI
Agent->>Tools: request tool by name
Tools->>ToolDef: invoke (toVercelAITool) with input
ToolDef->>ToolDef: execute logic → { output, metadata }
ToolDef-->>Agent: return result with metadata
Agent->>UI: emit message part containing output.metadata
UI->>UI: render using metadata fields
Note over MCP,Tools: MCP registration/invoke
MCP->>Tools: register tool via registerMcpTool
MCP->>ToolDef: invoke tool (source='mcp')
ToolDef-->>MCP: return { output, metadata }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
License Audit❌ Status: FAIL
Fail Reasons
Unresolved Packages
Weak Copyleft Packages (informational)
Resolved Packages (6)
|
This comment has been minimized.
This comment has been minimized.
brendan-kellam
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (5)
.env.development (1)
80-80: Move this debug override to.env.development.localinstead of committing it in.env.development.Line 80 is environment-specific behavior and is better kept in local overrides to avoid changing the shared default dev profile.
Based on learnings: Applies to .env.development.local : Use.env.development.localfor environment variable overrides instead of modifying.env.development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.development at line 80, The DEBUG_WRITE_CHAT_MESSAGES_TO_FILE debug override should not be committed in the shared .env.development; remove the line "DEBUG_WRITE_CHAT_MESSAGES_TO_FILE=true" from .env.development and add the same entry to .env.development.local instead, ensuring .env.development.local is listed in .gitignore so this environment-specific override remains local; update any docs or README that mention local env overrides if needed.packages/web/src/features/chat/components/chatThread/tools/shared.tsx (1)
126-134: Accessibility: copy action is not keyboard-accessible.The
eslint-disablesuppressesjsx-a11y/click-events-have-key-eventsandjsx-a11y/no-static-element-interactions, but this means keyboard users cannot trigger the copy action via the wrapper div. WhileCopyIconButtonitself may be focusable, thestopPropagationwrapper intercepts clicks but not keyboard events.Consider making the wrapper transparent to keyboard events or ensuring
CopyIconButtonhandles its own event propagation:♻️ Suggested approach
{onCopy && ( - // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions - <div onClick={(e) => e.stopPropagation()}> + <div + onClick={(e) => e.stopPropagation()} + onKeyDown={(e) => e.stopPropagation()} + > <CopyIconButton onCopy={onCopy} className="opacity-0 group-hover/header:opacity-100 transition-opacity" /> </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/chat/components/chatThread/tools/shared.tsx` around lines 126 - 134, The wrapper div around CopyIconButton prevents keyboard activation because it intercepts click events with onClick={(e) => e.stopPropagation()} and is not keyboard-accessible; remove the non-interactive wrapper or convert it to a keyboard-focusable element (e.g., a button or role="button" with onKeyDown handling) and ensure it calls e.stopPropagation() for both click and keyboard activation, or simply move the stopPropagation logic into CopyIconButton so the wrapper is not needed; update references around the onCopy prop and CopyIconButton usage to keep identical visual behavior while restoring keyboard event handling and removing the eslint-disable comments.packages/web/src/features/tools/index.ts (1)
1-8: Rename this barrel to a camelCase filename.If this shared export surface is staying, please avoid introducing a new
index.tsexception and give it a descriptive camelCase name instead.As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Files should use camelCase starting with a lowercase letter (e.g.,shareChatPopover.tsx,userAvatar.tsx,apiClient.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/tools/index.ts` around lines 1 - 8, The barrel file named index.ts in the tools feature should be renamed to a camelCase filename (e.g., toolsExports.ts or toolsBarrel.ts) to follow project naming rules; update the filename that currently re-exports readFile, listCommits, listRepos, searchCode, findSymbolReferences, findSymbolDefinitions, listTree, and adapters, and then update all import sites that currently import from the tools directory (which rely on the index.ts implicit resolution) to import from the new camelCase module name (ensure references to exports like readFile, listRepos, searchCode, etc. remain unchanged).packages/web/src/features/tools/listTree.ts (1)
3-5: Keep shared tree helpers out offeatures/mcp.
listTreeDefinitionlives in the shared tools layer, but it now depends on@/features/mcp/utilsfor generic tree/path helpers. That inverts the boundary this PR is trying to clean up and makes the tool registry transitively depend on an integration-specific package. Please move these helpers to a neutral shared module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/tools/listTree.ts` around lines 3 - 5, listTreeDefinition imports generic helpers from "@/features/mcp/utils" which inverts module boundaries; move the helpers buildTreeNodeIndex, joinTreePath, normalizeTreePath, and sortTreeEntries into a neutral shared module (e.g., a new shared utils under features/tools or a common utils package) and update listTreeDefinition to import them from that shared location instead of "@/features/mcp/utils"; ensure exports and any internal types/signatures remain unchanged and update other references to these helpers to the new module.packages/web/src/features/mcp/server.ts (1)
28-41: Consider movinglist_language_modelsto a shared tool definition for consistency.Everything else is now registered through
registerMcpTool(...); keeping this one inline leaves the MCP wiring partially divergent. Moving it intofeatures/toolswould keep one registration pattern and simplify reuse/testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/mcp/server.ts` around lines 28 - 41, Move the inline "list_language_models" tool registration into the shared tool definitions so MCP wiring is consistent: create a tool definition in the features/tools module (similar to other tools registered via registerMcpTool) that exposes the same behavior (calls getConfiguredLanguageModelsInfo and returns the JSON text payload), export it, and then replace the inline server.registerTool("list_language_models", ...) call with a registerMcpTool import/registration of that shared tool; ensure the exported tool name and behavior match the original list_language_models implementation and reuse getConfiguredLanguageModelsInfo for fetching models.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.development:
- Line 80: The file ends without a trailing newline which triggers
dotenv-linter's EndingBlankLine; open the .env.development file and add a final
blank line (newline character) after the last entry
DEBUG_WRITE_CHAT_MESSAGES_TO_FILE=true so the file ends with a newline
character.
In `@packages/web/next.config.mjs`:
- Around line 66-73: The turbopack rule for '*.txt' only covers dev; add an
equivalent webpack rule in the Next.js config's webpack property so production
builds (next build) use raw-loader for .txt imports: update the exported config
to include a webpack function that pushes a module.rules entry matching /\.txt$/
and uses 'raw-loader' (mirroring the turbopack loaders setting), and ensure the
loader is installed; locate the existing turbopack block (the turbopack.rules
'*.txt' entry) and add the corresponding webpack.module.rules rule in the same
next.config.mjs config object.
In
`@packages/web/src/features/chat/components/chatThread/tools/findSymbolDefinitionsToolComponent.tsx`:
- Around line 55-60: The list key for FileListItem is not unique across repos
because it uses file.fileName alone; update the map rendering in the
part.output.metadata.files loop to use a repo-qualified key (e.g., combine
file.repo and file.fileName or use `${file.repo || 'unknown'}:${file.fileName}`)
so keys are unique across repositories; change the key prop on the FileListItem
(and any other place relying on file.fileName as a key) to this combined
identifier while keeping path={file.fileName} and repoName={file.repo} intact.
In
`@packages/web/src/features/chat/components/chatThread/tools/listCommitsToolComponent.tsx`:
- Around line 26-28: The onCopy handler currently calls
navigator.clipboard.writeText(...) without awaiting or catching rejections;
update the onCopy definition in listCommitsToolComponent (the handler passed to
CopyIconButton) to be an async function that awaits
navigator.clipboard.writeText(part.output.output) and catches errors (e.g.,
try/catch) so it returns a boolean indicating success (true on success, false on
failure) and logs or handles the error rather than letting a rejected promise
become unhandled.
In
`@packages/web/src/features/chat/components/chatThread/tools/listReposToolComponent.tsx`:
- Around line 26-28: The current onCopy callback is synchronous (onCopy?: () =>
boolean) but clipboard.writeText is async; fix by updating the
ToolHeader/CopyIconButton contract to accept Promise<boolean> (change onCopy to
() => boolean | Promise<boolean>), update CopyIconButton to await the result,
handle errors and reflect success/failure before showing UI feedback, and then
change callers (e.g., listReposToolComponent, listCommitsToolComponent,
searchCodeToolComponent, readFileToolComponent, listTreeToolComponent,
findSymbolReferencesToolComponent, findSymbolDefinitionsToolComponent) to return
a Promise<boolean> that resolves true on successful
navigator.clipboard.writeText(...) and false on rejection so success is
determined after the async operation completes.
In
`@packages/web/src/features/chat/components/chatThread/tools/searchCodeToolComponent.tsx`:
- Around line 34-36: The onCopy callback currently returns true immediately
while navigator.clipboard.writeText(...) is async; update the handler used in
ToolHeader/CopyIconButton (onCopy) to either accept and return a
Promise<boolean> (change signature to onCopy?: () => Promise<boolean>) or wrap
the clipboard call to await it and return a boolean based on success/failure
(i.e., await navigator.clipboard.writeText(...); return true on success, catch
and return false on failure). Apply this same fix consistently in
searchCodeToolComponent.tsx and the other tool components listed (readFile,
listCommits, listTree, listRepos, findSymbolDefinitions, findSymbolReferences)
so CopyIconButton only shows success after the clipboard operation actually
completes.
In `@packages/web/src/features/mcp/server.ts`:
- Around line 12-26: The MCP server is missing registration for the
findSymbolDefinitionsDefinition tool, causing MCP clients to lack parity with
chat tools; in createMcpServer() import findSymbolDefinitionsDefinition from the
tools module and call registerMcpTool(server, findSymbolDefinitionsDefinition)
alongside the other registerMcpTool(...) calls so the MCP server exposes the
same symbol-finding tool as the chat surface.
In `@packages/web/src/features/mcp/utils.ts`:
- Line 3: Replace the runtime import of ListTreeEntry with a type-only import to
break the circular dependency: change the import that currently reads "import {
ListTreeEntry } from '@/features/tools/listTree'" to a type-only import "import
type { ListTreeEntry } from '@/features/tools/listTree'"; keep all usages of the
ListTreeEntry type unchanged (e.g., in functions or type annotations within
mcp/utils.ts) so only the import form is modified.
In `@packages/web/src/features/tools/findSymbolDefinitions.ts`:
- Around line 34-39: The symbol lookup is treating `symbol` as a regex pattern;
to fix it, escape the identifier before interpolating into the regex. In the
helper in codeNav/api.ts (the function that currently builds
"\\b${symbolName}\\b") wrap `symbolName` with `escapeStringRegexp(symbolName)`
(and import escapeStringRegexp if not present) so symbols like "foo.bar", "C++",
or "operator[]" are matched literally; this mirrors how `repoName` is handled in
the same helper and ensures findSearchBasedSymbolDefinitions /
findSymbolDefinitions use a literal-symbol match.
In `@packages/web/src/features/tools/listTree.ts`:
- Around line 63-65: The code enqueues directories into queue/queuedPaths before
applying includeDirectories, causing very wide levels passed to getTree; modify
listTree traversal (references: queue, queuedPaths, seenEntries, getTree,
includeDirectories) to batch or cap currentLevelPaths before each getTree call
so you never call getTree with an unbounded wide list: when building
currentLevelPaths from queue, partition it into chunks (or impose a max per-call
cap) and call getTree repeatedly per chunk, ensuring queuedPaths/seenEntries are
updated per-chunk and that includeDirectories filtering is applied before
enqueuing children so queued directory growth is limited independently from
entries length.
- Around line 77-81: The code calls getTree and then later treats a missing or
non-tree node as an empty directory; instead, after each getTree call (the one
using currentLevelPaths.filter(Boolean) and the later getTree at lines 89-91),
check the resolved node for the requested path (currentNode) and if the original
path was provided but currentNode is falsy or currentNode.type !== 'tree', throw
or return an explicit error (e.g., throw new Error(`path "${path}" not found or
is not a directory`)) so invalid paths fail fast rather than showing an empty
listing; update both call sites (the getTree invocation and the subsequent
getTree at 89-91) to perform this validation using the existing
treeResult/currentNode variables.
In `@packages/web/src/features/tools/readFile.ts`:
- Around line 57-106: The payload byte-cap logic is wrong: you only measured raw
line bytes (loop over lines -> slicedLines) but you later add line-number
prefixes and footers so the final output can exceed MAX_BYTES, and you don't
handle slicedLines.length === 0 (which makes lastReadLine = startLine - 1 and a
non-advancing offset). Fix readFile.ts by building the formatted line entries
(including the `${startLine + i}: ` prefixes and newline separators) and measure
Buffer.byteLength of the full output pieces (header, joined formatted lines, and
footer) against MAX_BYTES before committing a line to slicedLines; when slicing
would exceed MAX_BYTES set truncatedByBytes and stop. Also special-case
slicedLines.length === 0 to set lastReadLine = startLine - 1 (or better: leave
endLine = startLine - 1) and set nextOffset = startLine (or nextOffset =
startLine if nothing consumed) so the continuation offset advances or is stable,
and ensure metadata.isTruncated is true when the footer indicates truncation.
In `@packages/web/src/features/tools/searchCode.ts`:
- Around line 83-97: The current code appends raw filter strings for repos,
languages, filepaths, and ref into the query (see variables repos, languages,
filepaths, ref and the use of escapeStringRegexp) which allows spaces or
operators to break out of the intended filter; change this to build filters
structurally by creating a filter array (e.g., searchFilters) and pushing
properly escaped filter tokens for each type instead of string-splicing into
query, or at minimum run a filter-specific escaping/quoting function on each
language, filepath and ref value before joining; update the logic that currently
builds query with `repo:...`, `lang:...`, `file:...`, `rev:...` so it constructs
and joins safe filter tokens and then appends them to the main query passed to
search().
- Around line 44-48: The schema for limit currently allows negative and
non-integer values which then flow into options.matches; update the zod schema
and normalization so limit becomes a non-negative integer count before use:
change the z.number() for limit to enforce integers and a minimum (e.g.
.int().min(0)) or coerce/normalize by taking Math.floor and clamping between 0
and DEFAULT_SEARCH_LIMIT, then assign that normalized value to options.matches
(referencing the existing limit symbol and options.matches) so
decimals/negatives cannot reach the search call.
---
Nitpick comments:
In @.env.development:
- Line 80: The DEBUG_WRITE_CHAT_MESSAGES_TO_FILE debug override should not be
committed in the shared .env.development; remove the line
"DEBUG_WRITE_CHAT_MESSAGES_TO_FILE=true" from .env.development and add the same
entry to .env.development.local instead, ensuring .env.development.local is
listed in .gitignore so this environment-specific override remains local; update
any docs or README that mention local env overrides if needed.
In `@packages/web/src/features/chat/components/chatThread/tools/shared.tsx`:
- Around line 126-134: The wrapper div around CopyIconButton prevents keyboard
activation because it intercepts click events with onClick={(e) =>
e.stopPropagation()} and is not keyboard-accessible; remove the non-interactive
wrapper or convert it to a keyboard-focusable element (e.g., a button or
role="button" with onKeyDown handling) and ensure it calls e.stopPropagation()
for both click and keyboard activation, or simply move the stopPropagation logic
into CopyIconButton so the wrapper is not needed; update references around the
onCopy prop and CopyIconButton usage to keep identical visual behavior while
restoring keyboard event handling and removing the eslint-disable comments.
In `@packages/web/src/features/mcp/server.ts`:
- Around line 28-41: Move the inline "list_language_models" tool registration
into the shared tool definitions so MCP wiring is consistent: create a tool
definition in the features/tools module (similar to other tools registered via
registerMcpTool) that exposes the same behavior (calls
getConfiguredLanguageModelsInfo and returns the JSON text payload), export it,
and then replace the inline server.registerTool("list_language_models", ...)
call with a registerMcpTool import/registration of that shared tool; ensure the
exported tool name and behavior match the original list_language_models
implementation and reuse getConfiguredLanguageModelsInfo for fetching models.
In `@packages/web/src/features/tools/index.ts`:
- Around line 1-8: The barrel file named index.ts in the tools feature should be
renamed to a camelCase filename (e.g., toolsExports.ts or toolsBarrel.ts) to
follow project naming rules; update the filename that currently re-exports
readFile, listCommits, listRepos, searchCode, findSymbolReferences,
findSymbolDefinitions, listTree, and adapters, and then update all import sites
that currently import from the tools directory (which rely on the index.ts
implicit resolution) to import from the new camelCase module name (ensure
references to exports like readFile, listRepos, searchCode, etc. remain
unchanged).
In `@packages/web/src/features/tools/listTree.ts`:
- Around line 3-5: listTreeDefinition imports generic helpers from
"@/features/mcp/utils" which inverts module boundaries; move the helpers
buildTreeNodeIndex, joinTreePath, normalizeTreePath, and sortTreeEntries into a
neutral shared module (e.g., a new shared utils under features/tools or a common
utils package) and update listTreeDefinition to import them from that shared
location instead of "@/features/mcp/utils"; ensure exports and any internal
types/signatures remain unchanged and update other references to these helpers
to the new module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49e1a624-6438-4b46-a456-c43652448afa
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
.env.developmentpackages/shared/src/logger.tspackages/web/next.config.mjspackages/web/package.jsonpackages/web/src/features/chat/agent.tspackages/web/src/features/chat/components/chatThread/chatThreadListItem.tsxpackages/web/src/features/chat/components/chatThread/detailsCard.tsxpackages/web/src/features/chat/components/chatThread/tools/findSymbolDefinitionsToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/findSymbolReferencesToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listCommitsToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listReposToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listTreeToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/readFileToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/searchCodeToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/shared.tsxpackages/web/src/features/chat/constants.tspackages/web/src/features/chat/logger.tspackages/web/src/features/chat/tools.tspackages/web/src/features/chat/types.tspackages/web/src/features/chat/utils.tspackages/web/src/features/mcp/server.tspackages/web/src/features/mcp/types.tspackages/web/src/features/mcp/utils.tspackages/web/src/features/tools/adapters.tspackages/web/src/features/tools/findSymbolDefinitions.tspackages/web/src/features/tools/findSymbolDefinitions.txtpackages/web/src/features/tools/findSymbolReferences.tspackages/web/src/features/tools/findSymbolReferences.txtpackages/web/src/features/tools/index.tspackages/web/src/features/tools/listCommits.tspackages/web/src/features/tools/listCommits.txtpackages/web/src/features/tools/listRepos.tspackages/web/src/features/tools/listRepos.txtpackages/web/src/features/tools/listTree.tspackages/web/src/features/tools/listTree.txtpackages/web/src/features/tools/logger.tspackages/web/src/features/tools/readFile.tspackages/web/src/features/tools/readFile.txtpackages/web/src/features/tools/searchCode.tspackages/web/src/features/tools/searchCode.txtpackages/web/src/features/tools/types.tspackages/web/types.d.ts
💤 Files with no reviewable changes (1)
- packages/web/src/features/chat/constants.ts
| NODE_ENV=development | ||
| # SOURCEBOT_TENANCY_MODE=single | ||
|
|
||
| DEBUG_WRITE_CHAT_MESSAGES_TO_FILE=true No newline at end of file |
There was a problem hiding this comment.
Add a trailing newline at EOF.
dotenv-linter flagged EndingBlankLine on Line 80; please add a final blank line to satisfy the linter.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 80-80: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.development at line 80, The file ends without a trailing newline which
triggers dotenv-linter's EndingBlankLine; open the .env.development file and add
a final blank line (newline character) after the last entry
DEBUG_WRITE_CHAT_MESSAGES_TO_FILE=true so the file ends with a newline
character.
| turbopack: { | ||
| rules: { | ||
| '*.txt': { | ||
| loaders: ['raw-loader'], | ||
| as: '*.js', | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if webpack config for raw-loader exists elsewhere in the codebase
rg -n "raw-loader" --type=js --type=ts -g '!node_modules'
# Check for any existing webpack config modifications
ast-grep --pattern 'webpack: ($config) => { $$$ }'Repository: sourcebot-dev/sourcebot
Length of output: 49
🏁 Script executed:
# Examine the next.config.mjs file
cat -n packages/web/next.config.mjsRepository: sourcebot-dev/sourcebot
Length of output: 4920
🏁 Script executed:
# Search for .txt file imports
rg "import\s+.*\.txt" --type=js --type=ts --type=jsx --type=tsx
# Also check for require statements with .txt
rg "require\(.*\.txt" --type=js --type=ts
# Check if raw-loader is in package.json dependencies
cat packages/web/package.json | grep -A 5 -B 5 "raw-loader"Repository: sourcebot-dev/sourcebot
Length of output: 427
🏁 Script executed:
# Search for any .txt imports without type restrictions
rg "\.txt['\"]"
# Also list any .txt files in the repo
fd "\.txt$" packages/web --type fRepository: sourcebot-dev/sourcebot
Length of output: 2295
🏁 Script executed:
# Check if there's any webpack modification in the entire next.config.mjs
grep -n "webpack" packages/web/next.config.mjs
# Check if there are any other config files that might handle webpack
fd "webpack" packages/web --type f
# Check if there's anything in tsconfig or other config files
cat packages/web/tsconfig.json | head -30Repository: sourcebot-dev/sourcebot
Length of output: 616
🏁 Script executed:
# Check if there's a build or test script that might have caught this
cat packages/web/package.json | grep -A 10 '"scripts"'
# Also verify if there are any error suppression configs that might hide this
rg "suppress\|ignore.*raw-loader\|webpack.*rule" packages/webRepository: sourcebot-dev/sourcebot
Length of output: 834
Add webpack configuration for raw-loader to fix production builds.
The turbopack config at lines 66–73 only applies to next dev --turbopack. Production builds use webpack via next build, which has no rule for .txt files and will fail when importing them. This affects 7 files that import .txt files in production code.
🔧 Proposed fix
turbopack: {
rules: {
'*.txt': {
loaders: ['raw-loader'],
as: '*.js',
},
},
},
+
+ webpack: (config) => {
+ config.module.rules.push({
+ test: /\.txt$/,
+ use: 'raw-loader',
+ });
+ return config;
+ },📝 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.
| turbopack: { | |
| rules: { | |
| '*.txt': { | |
| loaders: ['raw-loader'], | |
| as: '*.js', | |
| }, | |
| }, | |
| }, | |
| turbopack: { | |
| rules: { | |
| '*.txt': { | |
| loaders: ['raw-loader'], | |
| as: '*.js', | |
| }, | |
| }, | |
| }, | |
| webpack: (config) => { | |
| config.module.rules.push({ | |
| test: /\.txt$/, | |
| use: 'raw-loader', | |
| }); | |
| return config; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/next.config.mjs` around lines 66 - 73, The turbopack rule for
'*.txt' only covers dev; add an equivalent webpack rule in the Next.js config's
webpack property so production builds (next build) use raw-loader for .txt
imports: update the exported config to include a webpack function that pushes a
module.rules entry matching /\.txt$/ and uses 'raw-loader' (mirroring the
turbopack loaders setting), and ensure the loader is installed; locate the
existing turbopack block (the turbopack.rules '*.txt' entry) and add the
corresponding webpack.module.rules rule in the same next.config.mjs config
object.
| {part.output.metadata.files.map((file) => { | ||
| return ( | ||
| <FileListItem | ||
| key={file.fileName} | ||
| path={file.fileName} | ||
| repoName={file.repository} | ||
| repoName={file.repo} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file to see the actual implementation
find . -type f -name "findSymbolDefinitionsToolComponent.tsx" | head -5Repository: sourcebot-dev/sourcebot
Length of output: 166
🏁 Script executed:
# Check if the file exists and read it
cat -n packages/web/src/features/chat/components/chatThread/tools/findSymbolDefinitionsToolComponent.tsx | head -100Repository: sourcebot-dev/sourcebot
Length of output: 3841
🏁 Script executed:
# Also check the structure around lines 55-60 specifically
sed -n '40,70p' packages/web/src/features/chat/components/chatThread/tools/findSymbolDefinitionsToolComponent.tsxRepository: sourcebot-dev/sourcebot
Length of output: 1583
🏁 Script executed:
# Find the type definition for FindSymbolDefinitionsToolUIPart
rg "FindSymbolDefinitionsToolUIPart" --type ts --type tsx -A 5 | head -50Repository: sourcebot-dev/sourcebot
Length of output: 94
🏁 Script executed:
# Search for where this tool is called or where the metadata.files structure is defined
rg "metadata.*files" --type ts --type tsx -B 3 -A 3 | head -60Repository: sourcebot-dev/sourcebot
Length of output: 94
🏁 Script executed:
# Look for the actual tool implementation that generates these results
fd -e ts -e tsx | xargs grep -l "FindSymbolDefinitions" | grep -v node_modulesRepository: sourcebot-dev/sourcebot
Length of output: 330
🏁 Script executed:
# Check the tools.ts file for type definitions
cat packages/web/src/features/chat/tools.tsRepository: sourcebot-dev/sourcebot
Length of output: 1748
🏁 Script executed:
# Check the findSymbolDefinitions.ts file
cat packages/web/src/features/tools/findSymbolDefinitions.tsRepository: sourcebot-dev/sourcebot
Length of output: 2167
🏁 Script executed:
# Check the FindSymbolFile type definition
rg "type FindSymbolFile" packages/web/src/features/tools/ -A 10Repository: sourcebot-dev/sourcebot
Length of output: 901
🏁 Script executed:
# Also check the actual implementation to see if multi-repo results are possible
rg "findSearchBasedSymbolDefinitions" -A 20 packages/web/src/features/ | head -60Repository: sourcebot-dev/sourcebot
Length of output: 5233
🏁 Script executed:
# Let's verify if the search can actually return multi-repo results
rg "FindRelatedSymbols" packages/web/src/features/codeNav/ -A 5 | head -30Repository: sourcebot-dev/sourcebot
Length of output: 2272
🏁 Script executed:
# Check if there's any filtering or scope limiting that prevents multi-repo results
rg "repository\|repositories" packages/web/src/features/codeNav/api.ts -B 2 -A 2Repository: sourcebot-dev/sourcebot
Length of output: 49
🏁 Script executed:
# Let's look at the full API implementation to understand multi-repo behavior
cat packages/web/src/features/codeNav/api.ts | head -100Repository: sourcebot-dev/sourcebot
Length of output: 3386
🏁 Script executed:
# Check the response schema to see if it includes repository field
cat packages/web/src/features/codeNav/types.tsRepository: sourcebot-dev/sourcebot
Length of output: 1284
Use a repo-qualified key for definition hits.
file.fileName alone is not unique when results span multiple repositories. Since repoName is optional and the search can return matches across all repositories, files like src/index.ts will collide across different repos. This causes React to reuse the wrong list item DOM nodes. Combine both repo and fileName in the key to ensure uniqueness.
Suggested fix
{part.output.metadata.files.map((file) => {
return (
<FileListItem
- key={file.fileName}
+ key={`${file.repo}:${file.fileName}`}
path={file.fileName}
repoName={file.repo}
/>
)
})}📝 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.
| {part.output.metadata.files.map((file) => { | |
| return ( | |
| <FileListItem | |
| key={file.fileName} | |
| path={file.fileName} | |
| repoName={file.repository} | |
| repoName={file.repo} | |
| {part.output.metadata.files.map((file) => { | |
| return ( | |
| <FileListItem | |
| key={`${file.repo}:${file.fileName}`} | |
| path={file.fileName} | |
| repoName={file.repo} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/web/src/features/chat/components/chatThread/tools/findSymbolDefinitionsToolComponent.tsx`
around lines 55 - 60, The list key for FileListItem is not unique across repos
because it uses file.fileName alone; update the map rendering in the
part.output.metadata.files loop to use a repo-qualified key (e.g., combine
file.repo and file.fileName or use `${file.repo || 'unknown'}:${file.fileName}`)
so keys are unique across repositories; change the key prop on the FileListItem
(and any other place relying on file.fileName as a key) to this combined
identifier while keeping path={file.fileName} and repoName={file.repo} intact.
| const onCopy = part.state === 'output-available' && !isServiceError(part.output) | ||
| ? () => { navigator.clipboard.writeText(part.output.output); return true; } | ||
| : undefined; |
There was a problem hiding this comment.
Clipboard write lacks error handling.
navigator.clipboard.writeText() returns a Promise that can reject (e.g., if the document isn't focused or permissions are denied). The current code doesn't await or catch errors, which could cause unhandled promise rejections.
🔧 Proposed fix
const onCopy = part.state === 'output-available' && !isServiceError(part.output)
- ? () => { navigator.clipboard.writeText(part.output.output); return true; }
+ ? () => {
+ navigator.clipboard.writeText(part.output.output).catch(() => {});
+ return true;
+ }
: undefined;Alternatively, if CopyIconButton supports async handlers, consider returning the promise result.
📝 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.
| const onCopy = part.state === 'output-available' && !isServiceError(part.output) | |
| ? () => { navigator.clipboard.writeText(part.output.output); return true; } | |
| : undefined; | |
| const onCopy = part.state === 'output-available' && !isServiceError(part.output) | |
| ? () => { | |
| navigator.clipboard.writeText(part.output.output).catch(() => {}); | |
| return true; | |
| } | |
| : undefined; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/web/src/features/chat/components/chatThread/tools/listCommitsToolComponent.tsx`
around lines 26 - 28, The onCopy handler currently calls
navigator.clipboard.writeText(...) without awaiting or catching rejections;
update the onCopy definition in listCommitsToolComponent (the handler passed to
CopyIconButton) to be an async function that awaits
navigator.clipboard.writeText(part.output.output) and catches errors (e.g.,
try/catch) so it returns a boolean indicating success (true on success, false on
failure) and logs or handles the error rather than letting a rejected promise
become unhandled.
| const onCopy = part.state === 'output-available' && !isServiceError(part.output) | ||
| ? () => { navigator.clipboard.writeText(part.output.output); return true; } | ||
| : undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file and see the code at lines 26-28
cat -n packages/web/src/features/chat/components/chatThread/tools/listReposToolComponent.tsx | head -70Repository: sourcebot-dev/sourcebot
Length of output: 3813
🏁 Script executed:
# Also find the ToolHeader component to understand what it expects for the onCopy prop
fd -t f "ToolHeader" packages/web/src --type typescriptRepository: sourcebot-dev/sourcebot
Length of output: 244
🏁 Script executed:
# Search for other onCopy handlers in tool components
rg "onCopy" packages/web/src/features/chat/components/chatThread/tools/ -A 3Repository: sourcebot-dev/sourcebot
Length of output: 9182
🏁 Script executed:
rg "CopyIconButton" packages/web/src/features/chat/components/chatThread/tools/shared.tsx -A 20Repository: sourcebot-dev/sourcebot
Length of output: 1486
🏁 Script executed:
# Find the CopyIconButton definition
fd -name "*copy*" -o -name "*button*" packages/web/src | head -20Repository: sourcebot-dev/sourcebot
Length of output: 297
🏁 Script executed:
rg "export.*CopyIconButton" packages/web/src -A 15Repository: sourcebot-dev/sourcebot
Length of output: 1515
The suggested fix is incompatible with the current ToolHeader/CopyIconButton architecture.
The underlying concern is valid: navigator.clipboard.writeText() is async and returning true synchronously means success is reported before the operation completes, and failures go unhandled. However, the proposed solution won't work.
The ToolHeader expects onCopy?: () => boolean; and CopyIconButton immediately calls const success = onCopy(); and checks it synchronously. Making the callback async () => Promise<boolean> would break the type contract and cause runtime errors.
The pattern also appears in listCommitsToolComponent, searchCodeToolComponent, readFileToolComponent, listTreeToolComponent, findSymbolReferencesToolComponent, and findSymbolDefinitionsToolComponent.
To fix this properly, either:
- Refactor
ToolHeader/CopyIconButtonto accept and handlePromise<boolean>or async callbacks, or - Keep callbacks synchronous but don't rely on awaited clipboard completion for success feedback
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/web/src/features/chat/components/chatThread/tools/listReposToolComponent.tsx`
around lines 26 - 28, The current onCopy callback is synchronous (onCopy?: () =>
boolean) but clipboard.writeText is async; fix by updating the
ToolHeader/CopyIconButton contract to accept Promise<boolean> (change onCopy to
() => boolean | Promise<boolean>), update CopyIconButton to await the result,
handle errors and reflect success/failure before showing UI feedback, and then
change callers (e.g., listReposToolComponent, listCommitsToolComponent,
searchCodeToolComponent, readFileToolComponent, listTreeToolComponent,
findSymbolReferencesToolComponent, findSymbolDefinitionsToolComponent) to return
a Promise<boolean> that resolves true on successful
navigator.clipboard.writeText(...) and false on rejection so success is
determined after the async operation completes.
| const queue: Array<{ path: string; depth: number }> = [{ path: normalizedPath, depth: 0 }]; | ||
| const queuedPaths = new Set<string>([normalizedPath]); | ||
| const seenEntries = new Set<string>(); |
There was a problem hiding this comment.
Chunk wide directory levels before calling getTree.
Directories are enqueued before the includeDirectories filter runs, so includeDirectories=false can still accumulate a very large queue without increasing entries.length. The next iteration then sends that whole level to one getTree call, which can exceed argv limits and fail on wide repos. Please batch currentLevelPaths or cap queued directories independently of returned entries.
Also applies to: 69-81, 99-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/tools/listTree.ts` around lines 63 - 65, The code
enqueues directories into queue/queuedPaths before applying includeDirectories,
causing very wide levels passed to getTree; modify listTree traversal
(references: queue, queuedPaths, seenEntries, getTree, includeDirectories) to
batch or cap currentLevelPaths before each getTree call so you never call
getTree with an unbounded wide list: when building currentLevelPaths from queue,
partition it into chunks (or impose a max per-call cap) and call getTree
repeatedly per chunk, ensuring queuedPaths/seenEntries are updated per-chunk and
that includeDirectories filtering is applied before enqueuing children so queued
directory growth is limited independently from entries length.
| const treeResult = await getTree({ | ||
| repoName: repo, | ||
| revisionName: ref, | ||
| paths: currentLevelPaths.filter(Boolean), | ||
| }, { source: context.source }); |
There was a problem hiding this comment.
Fail fast when path is missing or points to a file.
A bad path currently falls through !currentNode || currentNode.type !== 'tree' and returns an empty listing, which makes invalid input look like an empty directory instead of an error.
Suggested fix
for (const currentPath of currentLevelPaths) {
const currentNode = currentPath === '' ? treeResult.tree : treeNodeIndex.get(currentPath);
- if (!currentNode || currentNode.type !== 'tree') continue;
+ if (!currentNode) {
+ throw new Error(`Path "${currentPath}" not found.`);
+ }
+ if (currentNode.type !== 'tree') {
+ throw new Error(`Path "${currentPath}" is not a directory.`);
+ }
for (const child of currentNode.children) {Also applies to: 89-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/tools/listTree.ts` around lines 77 - 81, The code
calls getTree and then later treats a missing or non-tree node as an empty
directory; instead, after each getTree call (the one using
currentLevelPaths.filter(Boolean) and the later getTree at lines 89-91), check
the resolved node for the requested path (currentNode) and if the original path
was provided but currentNode is falsy or currentNode.type !== 'tree', throw or
return an explicit error (e.g., throw new Error(`path "${path}" not found or is
not a directory`)) so invalid paths fail fast rather than showing an empty
listing; update both call sites (the getTree invocation and the subsequent
getTree at 89-91) to perform this validation using the existing
treeResult/currentNode variables.
| const lines = fileSource.source.split('\n'); | ||
| const start = (offset ?? 1) - 1; | ||
| const end = start + Math.min(limit ?? READ_FILES_MAX_LINES, READ_FILES_MAX_LINES); | ||
|
|
||
| let bytes = 0; | ||
| let truncatedByBytes = false; | ||
| const slicedLines: string[] = []; | ||
| for (const raw of lines.slice(start, end)) { | ||
| const line = raw.length > MAX_LINE_LENGTH ? raw.substring(0, MAX_LINE_LENGTH) + MAX_LINE_SUFFIX : raw; | ||
| const size = Buffer.byteLength(line, 'utf-8') + (slicedLines.length > 0 ? 1 : 0); | ||
| if (bytes + size > MAX_BYTES) { | ||
| truncatedByBytes = true; | ||
| break; | ||
| } | ||
| slicedLines.push(line); | ||
| bytes += size; | ||
| } | ||
|
|
||
| const truncatedByLines = end < lines.length; | ||
| const startLine = (offset ?? 1); | ||
| const lastReadLine = startLine + slicedLines.length - 1; | ||
| const nextOffset = lastReadLine + 1; | ||
|
|
||
| let output = [ | ||
| `<repo>${fileSource.repo}</repo>`, | ||
| `<path>${fileSource.path}</path>`, | ||
| '<content>\n' | ||
| ].join('\n'); | ||
|
|
||
| output += slicedLines.map((line, i) => `${startLine + i}: ${line}`).join('\n'); | ||
|
|
||
| if (truncatedByBytes) { | ||
| output += `\n\n(Output capped at ${MAX_BYTES_LABEL}. Showing lines ${startLine}-${lastReadLine} of ${lines.length}. Use offset=${nextOffset} to continue.)`; | ||
| } else if (truncatedByLines) { | ||
| output += `\n\n(Showing lines ${startLine}-${lastReadLine} of ${lines.length}. Use offset=${nextOffset} to continue.)`; | ||
| } else { | ||
| output += `\n\n(End of file - ${lines.length} lines total)`; | ||
| } | ||
|
|
||
| output += `\n</content>`; | ||
|
|
||
| const metadata: ReadFileMetadata = { | ||
| path: fileSource.path, | ||
| repo: fileSource.repo, | ||
| language: fileSource.language, | ||
| startLine, | ||
| endLine: lastReadLine, | ||
| isTruncated: truncatedByBytes || truncatedByLines, | ||
| revision, | ||
| }; |
There was a problem hiding this comment.
The truncation math doesn't match the payload you actually emit.
Lines 61-73 only count raw line bytes, but Lines 80-96 also add line-number prefixes and footer text, so the returned payload can still exceed the advertised 5KB cap. Also, any empty-slice path (for example, an offset past EOF or the first line being rejected by the byte cap) makes lastReadLine = startLine - 1, which yields invalid metadata and a non-advancing continuation offset. Please base the cap on the fully formatted payload and special-case slicedLines.length === 0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/tools/readFile.ts` around lines 57 - 106, The
payload byte-cap logic is wrong: you only measured raw line bytes (loop over
lines -> slicedLines) but you later add line-number prefixes and footers so the
final output can exceed MAX_BYTES, and you don't handle slicedLines.length === 0
(which makes lastReadLine = startLine - 1 and a non-advancing offset). Fix
readFile.ts by building the formatted line entries (including the `${startLine +
i}: ` prefixes and newline separators) and measure Buffer.byteLength of the full
output pieces (header, joined formatted lines, and footer) against MAX_BYTES
before committing a line to slicedLines; when slicing would exceed MAX_BYTES set
truncatedByBytes and stop. Also special-case slicedLines.length === 0 to set
lastReadLine = startLine - 1 (or better: leave endLine = startLine - 1) and set
nextOffset = startLine (or nextOffset = startLine if nothing consumed) so the
continuation offset advances or is stable, and ensure metadata.isTruncated is
true when the footer indicates truncation.
| limit: z | ||
| .number() | ||
| .default(DEFAULT_SEARCH_LIMIT) | ||
| .describe(`Maximum number of matches to return (default: ${DEFAULT_SEARCH_LIMIT})`) | ||
| .optional(), |
There was a problem hiding this comment.
Validate limit as an actual count.
limit currently accepts negatives and decimals, and those values flow straight into options.matches. Since this is model-facing input, it should be normalized before the search call.
🛠️ Suggested diff
limit: z
.number()
+ .int()
+ .positive()
.default(DEFAULT_SEARCH_LIMIT)
- .describe(`Maximum number of matches to return (default: ${DEFAULT_SEARCH_LIMIT})`)
- .optional(),
+ .describe(`Maximum number of matches to return (default: ${DEFAULT_SEARCH_LIMIT})`),📝 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.
| limit: z | |
| .number() | |
| .default(DEFAULT_SEARCH_LIMIT) | |
| .describe(`Maximum number of matches to return (default: ${DEFAULT_SEARCH_LIMIT})`) | |
| .optional(), | |
| limit: z | |
| .number() | |
| .int() | |
| .positive() | |
| .default(DEFAULT_SEARCH_LIMIT) | |
| .describe(`Maximum number of matches to return (default: ${DEFAULT_SEARCH_LIMIT})`), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/tools/searchCode.ts` around lines 44 - 48, The
schema for limit currently allows negative and non-integer values which then
flow into options.matches; update the zod schema and normalization so limit
becomes a non-negative integer count before use: change the z.number() for limit
to enforce integers and a minimum (e.g. .int().min(0)) or coerce/normalize by
taking Math.floor and clamping between 0 and DEFAULT_SEARCH_LIMIT, then assign
that normalized value to options.matches (referencing the existing limit symbol
and options.matches) so decimals/negatives cannot reach the search call.
| if (repos.length > 0) { | ||
| query += ` (repo:${repos.map(id => escapeStringRegexp(id)).join(' or repo:')})`; | ||
| } | ||
|
|
||
| if (languages.length > 0) { | ||
| query += ` (lang:${languages.join(' or lang:')})`; | ||
| } | ||
|
|
||
| if (filepaths.length > 0) { | ||
| query += ` (file:${filepaths.join(' or file:')})`; | ||
| } | ||
|
|
||
| if (ref) { | ||
| query += ` (rev:${ref})`; | ||
| } |
There was a problem hiding this comment.
Build these filters structurally instead of splicing raw strings into the query syntax.
Lines 83-97 append languages, filepaths, and ref directly into a query that search() reparses. Values with spaces or query operators can change the parsed AST instead of staying inside their filter, which can broaden the search outside the intended repo/path/ref scope. Prefer constructing the filter IR directly, or at minimum use filter-specific escaping before concatenation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/tools/searchCode.ts` around lines 83 - 97, The
current code appends raw filter strings for repos, languages, filepaths, and ref
into the query (see variables repos, languages, filepaths, ref and the use of
escapeStringRegexp) which allows spaces or operators to break out of the
intended filter; change this to build filters structurally by creating a filter
array (e.g., searchFilters) and pushing properly escaped filter tokens for each
type instead of string-splicing into query, or at minimum run a filter-specific
escaping/quoting function on each language, filepath and ref value before
joining; update the logic that currently builds query with `repo:...`,
`lang:...`, `file:...`, `rev:...` so it constructs and joins safe filter tokens
and then appends them to the main query passed to search().
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/features/mcp/server.ts`:
- Around line 79-81: The tool definition for ask_codebase is incorrectly marked
as read-only; locate the ask_codebase tool entry (the object with annotations: {
readOnlyHint: true }) and change the annotation to reflect that it has side
effects by removing readOnlyHint or setting it to false so MCP clients won't
treat it as a read-only tool; update the annotations on the ask_codebase
definition in server.ts accordingly and ensure any related comment or metadata
reflects that it creates a persisted chat/research session (chatUrl).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8101998d-123e-4f9c-a09d-8b4f6b8a6da6
📒 Files selected for processing (3)
CHANGELOG.mdpackages/web/src/features/chat/components/chatThread/tools/listCommitsToolComponent.tsxpackages/web/src/features/mcp/server.ts
| annotations: { | ||
| readOnlyHint: true, | ||
| } |
There was a problem hiding this comment.
ask_codebase is incorrectly marked read-only.
Line 80 sets readOnlyHint: true, but this tool creates a persisted chat/research session (chatUrl), so it has side effects. This can cause incorrect tool gating in MCP clients.
Proposed fix
annotations: {
- readOnlyHint: true,
+ readOnlyHint: false,
}📝 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.
| annotations: { | |
| readOnlyHint: true, | |
| } | |
| annotations: { | |
| readOnlyHint: false, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/mcp/server.ts` around lines 79 - 81, The tool
definition for ask_codebase is incorrectly marked as read-only; locate the
ask_codebase tool entry (the object with annotations: { readOnlyHint: true })
and change the annotation to reflect that it has side effects by removing
readOnlyHint or setting it to false so MCP clients won't treat it as a read-only
tool; update the annotations on the ask_codebase definition in server.ts
accordingly and ensure any related comment or metadata reflects that it creates
a persisted chat/research session (chatUrl).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }, context) => { | ||
| logger.debug('grep', { pattern, path, include, repo, ref, limit }); | ||
|
|
||
| const quotedPattern = `"${pattern.replace(/"/g, '\\"')}"`; |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 hours ago
In general, when escaping a string for inclusion inside double quotes in a query language that uses backslash escapes, you must escape backslashes first, then escape double quotes, so that existing escape sequences cannot “break out” of the intended encoding. Using String.replace with a global regular expression is fine if all required characters are handled.
The best minimal fix here is to perform two replace operations: first escape all backslashes (\ → \\), then escape all double quotes (" → \"). This keeps existing functionality (double quotes are still escaped) and adds the missing backslash handling. We do not need new imports; the standard string methods suffice.
Concretely, in packages/web/src/features/tools/grep.ts, update line 74 so that quotedPattern is built from a version of pattern where backslashes and double quotes are both escaped. For example:
const escapedPattern = pattern.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
const quotedPattern = `"${escapedPattern}"`;This ensures all backslashes are preserved correctly inside the quoted string and prevents malformed escape sequences. No other parts of the file need to change.
| @@ -71,7 +71,8 @@ | ||
| }, context) => { | ||
| logger.debug('grep', { pattern, path, include, repo, ref, limit }); | ||
|
|
||
| const quotedPattern = `"${pattern.replace(/"/g, '\\"')}"`; | ||
| const escapedPattern = pattern.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); | ||
| const quotedPattern = `"${escapedPattern}"`; | ||
| let query = quotedPattern; | ||
|
|
||
| if (path) { |
| } | ||
|
|
||
| function buildRipgrepCommand({ pattern, path, include }: SearchInput): string { | ||
| const parts = ['rg', `"${pattern.replace(/"/g, '\\"')}"`]; |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
In general, the problem arises because the code tries to escape double quotes inside pattern by prefixing them with a backslash, but it does not escape existing backslashes. Proper backslash-based escaping for use inside double quotes should first escape backslashes, then escape double quotes. That way any \" in the input becomes \\" in the final string, preserving the intended literal characters when interpreted.
The best fix here is to replace the raw pattern.replace(/"/g, '\\"') logic with a small helper that correctly escapes both backslashes and double quotes in the order: backslashes first, then quotes. We only need this when embedding pattern into "..." for the ripgrep and zoekt pieces. This keeps existing functionality (printing a suggested command/query) while making escaping robust. Concretely:
- Add a helper
escapeDoubleQuotednear the other helper functions. - Implement it as:
s.replace(/\\/g, '\\\\').replace(/"/g, '\\"')
- Use this helper in:
buildRipgrepCommandline 25 for thepatternportion.buildZoektQueryline 32 for thepatternportion.
No new imports are needed; this uses only built-in string methods and regexes.
| @@ -14,6 +14,11 @@ | ||
| include?: string; // glob for filenames, e.g. "*.ts" or "**/*.{ts,tsx}" | ||
| } | ||
|
|
||
| function escapeDoubleQuoted(s: string): string { | ||
| // Escape backslashes first, then double quotes, for safe use inside "..." | ||
| return s.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); | ||
| } | ||
|
|
||
| function globToFileRegexp(glob: string): string { | ||
| const re = globToRegexp(glob, { extended: true, globstar: true }); | ||
| // Strip ^ anchor — Sourcebot file paths include the full repo-relative path, | ||
| @@ -22,14 +27,14 @@ | ||
| } | ||
|
|
||
| function buildRipgrepCommand({ pattern, path, include }: SearchInput): string { | ||
| const parts = ['rg', `"${pattern.replace(/"/g, '\\"')}"`]; | ||
| const parts = ['rg', `"${escapeDoubleQuoted(pattern)}"`]; | ||
| if (path) parts.push(path); | ||
| if (include) parts.push(`--glob "${include}"`); | ||
| return parts.join(' '); | ||
| } | ||
|
|
||
| function buildZoektQuery({ pattern, path, include }: SearchInput): string { | ||
| const parts: string[] = [`"${pattern.replace(/"/g, '\\"')}"`]; | ||
| const parts: string[] = [`"${escapeDoubleQuoted(pattern)}"`]; | ||
|
|
||
| if (path) { | ||
| parts.push(`file:${escapeStringRegexp(path)}`); |
| } | ||
|
|
||
| function buildZoektQuery({ pattern, path, include }: SearchInput): string { | ||
| const parts: string[] = [`"${pattern.replace(/"/g, '\\"')}"`]; |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
In general, when manually escaping characters to embed a string inside double quotes, escape both the quote character and backslashes so that no existing backslash can “undo” or alter the escaping you add. This avoids cases where \" in the original input becomes an actual quote boundary or where \ before another character forms an unintended escape sequence.
For this file, the most targeted fix is to change the escaping logic for pattern in buildZoektQuery (line 32) so that it first escapes backslashes, then escapes double quotes. We should not change the externally imported escape-string-regexp behavior or any other semantics. Concretely:
-
In
buildZoektQuery, replace
const parts: string[] = [\"${pattern.replace(/"/g, '\"')}"`];`
with a two-step escaping that handles backslashes and double quotes, e.g.:const escapedPattern = pattern.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); const parts: string[] = [`"${escapedPattern}"`];
-
Keep everything else in the file as-is.
This requires no new methods or imports; it uses only String.prototype.replace with proper global regexes.
| @@ -29,7 +29,8 @@ | ||
| } | ||
|
|
||
| function buildZoektQuery({ pattern, path, include }: SearchInput): string { | ||
| const parts: string[] = [`"${pattern.replace(/"/g, '\\"')}"`]; | ||
| const escapedPattern = pattern.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); | ||
| const parts: string[] = [`"${escapedPattern}"`]; | ||
|
|
||
| if (path) { | ||
| parts.push(`file:${escapeStringRegexp(path)}`); |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/web/src/features/mcp/server.ts (1)
79-81:⚠️ Potential issue | 🟠 Major
ask_codebaseis still misannotated as read-only.Line [80] remains
readOnlyHint: true, but this tool creates/persists a research session and returnschatUrl, so MCP clients may apply incorrect gating behavior.Suggested fix
annotations: { - readOnlyHint: true, + readOnlyHint: false, + idempotentHint: false, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/mcp/server.ts` around lines 79 - 81, The annotations for the ask_codebase tool are incorrect: change the annotations object used for ask_codebase (currently containing readOnlyHint: true) to reflect that this tool creates/persists a research session and returns chatUrl—remove or set readOnlyHint to false so MCP clients don't treat it as read-only; update the annotations for ask_codebase accordingly and ensure any documentation/comments near ask_codebase mention that it returns chatUrl and persists state.
🧹 Nitpick comments (2)
packages/web/tools/globToRegexpPlayground.ts (1)
17-43: Prefer reusing the shared grep query builders to avoid behavior drift.This playground duplicates query-construction logic. Given the ongoing
search_codeinterface churn in this PR, importing shared builders frompackages/web/src/features/tools/grep.tswill keep examples aligned with production behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/tools/globToRegexpPlayground.ts` around lines 17 - 43, This file duplicates query-builder logic (globToFileRegexp, buildRipgrepCommand, buildZoektQuery); replace these local implementations by importing and reusing the shared builders from the canonical grep module (use the exported buildRipgrepCommand and buildZoektQuery — and globToFileRegexp if exported — instead of redefining them), update calls to use the imported functions and remove the duplicate functions/escapeStringRegexp usage so the playground stays aligned with production behavior.packages/web/src/features/chat/components/chatThread/tools/listTreeToolComponent.tsx (1)
53-55: Prefer path-based keys over array indices.Line [54] uses
key={index}. A stable key likeentry.pathavoids subtle UI glitches when entry order/shape changes.Suggested refactor
- {part.output.metadata.entries.map((entry, index) => ( - <div key={index} className="flex items-center gap-2 text-sm" style={{ paddingLeft: `${(entry.depth - 1) * 12}px` }}> + {part.output.metadata.entries.map((entry) => ( + <div key={entry.path} className="flex items-center gap-2 text-sm" style={{ paddingLeft: `${(entry.depth - 1) * 12}px` }}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/chat/components/chatThread/tools/listTreeToolComponent.tsx` around lines 53 - 55, Replace the unstable index-based key in the list rendering with a stable path-based key: inside the map over part.output.metadata.entries in listTreeToolComponent (the arrow callback that uses entry.depth and entry.type), change key={index} to use a unique persistent identifier such as key={entry.path} (or a concatenation like `${entry.path}-${entry.type}` if needed) so React can track items reliably when order/shape changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/web/src/features/chat/components/chatThread/tools/findSymbolReferencesToolComponent.tsx`:
- Around line 52-57: The list item keys for reference results use only
file.fileName which can collide across repos; update the key in the map over
part.output.metadata.files (the block that renders FileListItem) to use a stable
unique identifier combining repo and path (e.g., `${file.repo}:${file.fileName}`
or a dedicated unique id if available) and ensure the FileListItem key prop uses
that combined value so React can properly reconcile items.
In
`@packages/web/src/features/chat/components/chatThread/tools/grepToolComponent.tsx`:
- Around line 58-63: The file list map uses file.fileName as the React key which
can collide across repos; update the mapping in part.output.metadata.files to
use a repo-qualified key (e.g., combine file.repo and file.fileName) for the
FileListItem key prop so each row is uniquely identified across
repositories—modify the code that renders <FileListItem key={...} ...> to use a
composite key like `${file.repo}-${file.fileName}` (or another unique repo+path
identifier) and keep path={file.fileName} and repoName={file.repo} unchanged.
In `@packages/web/src/features/tools/grep.ts`:
- Around line 75-92: The query construction is vulnerable because pattern is
only escaping quotes (not backslashes) when building quotedPattern and ref is
interpolated unescaped into the rev clause; update the logic that builds
quotedPattern to first escape backslashes and quotes (e.g., replace "\" with
"\\\\" then "\"" with "\\\"") before surrounding with quotes, and
sanitize/ref-escape the ref value before interpolating (use the existing
escapeStringRegexp or equivalent safe-quoting function) when constructing the
`(rev:...)` fragment; keep use of escapeStringRegexp for repo/path and
globToFileRegexp for include as-is.
In `@packages/web/tools/globToRegexpPlayground.ts`:
- Around line 24-27: The pattern quoting currently only escapes double quotes
which breaks when the pattern contains backslashes; update buildRipgrepCommand's
pattern handling to first escape backslashes (replace \ with \\) and then escape
double quotes (replace " with \\"), e.g. transform pattern via
pattern.replace(/\\/g,'\\\\').replace(/"/g,'\\"') before wrapping in quotes;
apply the same two-step escaping to the other quoted-pattern builders in this
file (the builders around lines 31-33) so backslashes and quotes are both
handled.
---
Duplicate comments:
In `@packages/web/src/features/mcp/server.ts`:
- Around line 79-81: The annotations for the ask_codebase tool are incorrect:
change the annotations object used for ask_codebase (currently containing
readOnlyHint: true) to reflect that this tool creates/persists a research
session and returns chatUrl—remove or set readOnlyHint to false so MCP clients
don't treat it as read-only; update the annotations for ask_codebase accordingly
and ensure any documentation/comments near ask_codebase mention that it returns
chatUrl and persists state.
---
Nitpick comments:
In
`@packages/web/src/features/chat/components/chatThread/tools/listTreeToolComponent.tsx`:
- Around line 53-55: Replace the unstable index-based key in the list rendering
with a stable path-based key: inside the map over part.output.metadata.entries
in listTreeToolComponent (the arrow callback that uses entry.depth and
entry.type), change key={index} to use a unique persistent identifier such as
key={entry.path} (or a concatenation like `${entry.path}-${entry.type}` if
needed) so React can track items reliably when order/shape changes.
In `@packages/web/tools/globToRegexpPlayground.ts`:
- Around line 17-43: This file duplicates query-builder logic (globToFileRegexp,
buildRipgrepCommand, buildZoektQuery); replace these local implementations by
importing and reusing the shared builders from the canonical grep module (use
the exported buildRipgrepCommand and buildZoektQuery — and globToFileRegexp if
exported — instead of redefining them), update calls to use the imported
functions and remove the duplicate functions/escapeStringRegexp usage so the
playground stays aligned with production behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f68f320-7b3a-441d-b7ec-8cd6515f4eef
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
packages/web/package.jsonpackages/web/src/features/chat/agent.tspackages/web/src/features/chat/components/chatThread/detailsCard.tsxpackages/web/src/features/chat/components/chatThread/tools/findSymbolDefinitionsToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/findSymbolReferencesToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/grepToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listCommitsToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listReposToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listTreeToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/readFileToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/shared.tsxpackages/web/src/features/chat/tools.tspackages/web/src/features/mcp/server.tspackages/web/src/features/tools/grep.tspackages/web/src/features/tools/grep.txtpackages/web/src/features/tools/index.tspackages/web/tools/globToRegexpPlayground.ts
✅ Files skipped from review due to trivial changes (1)
- packages/web/src/features/tools/grep.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/web/src/features/tools/index.ts
- packages/web/src/features/chat/components/chatThread/tools/listCommitsToolComponent.tsx
- packages/web/src/features/chat/components/chatThread/tools/findSymbolDefinitionsToolComponent.tsx
- packages/web/src/features/chat/components/chatThread/tools/listReposToolComponent.tsx
| {part.output.metadata.files.map((file) => { | ||
| return ( | ||
| <FileListItem | ||
| key={file.fileName} | ||
| path={file.fileName} | ||
| repoName={file.repository} | ||
| repoName={file.repo} |
There was a problem hiding this comment.
Use a stable unique key for reference results.
Line [55] keys rows by file.fileName only. If two repos contain the same path, keys collide and React can reconcile incorrectly.
Suggested fix
- <FileListItem
- key={file.fileName}
+ <FileListItem
+ key={`${file.repo}:${file.fileName}`}
path={file.fileName}
repoName={file.repo}
/>📝 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.
| {part.output.metadata.files.map((file) => { | |
| return ( | |
| <FileListItem | |
| key={file.fileName} | |
| path={file.fileName} | |
| repoName={file.repository} | |
| repoName={file.repo} | |
| {part.output.metadata.files.map((file) => { | |
| return ( | |
| <FileListItem | |
| key={`${file.repo}:${file.fileName}`} | |
| path={file.fileName} | |
| repoName={file.repo} | |
| /> | |
| ); | |
| })} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/web/src/features/chat/components/chatThread/tools/findSymbolReferencesToolComponent.tsx`
around lines 52 - 57, The list item keys for reference results use only
file.fileName which can collide across repos; update the key in the map over
part.output.metadata.files (the block that renders FileListItem) to use a stable
unique identifier combining repo and path (e.g., `${file.repo}:${file.fileName}`
or a dedicated unique id if available) and ensure the FileListItem key prop uses
that combined value so React can properly reconcile items.
| {part.output.metadata.files.map((file) => { | ||
| return ( | ||
| <FileListItem | ||
| key={file.fileName} | ||
| path={file.fileName} | ||
| repoName={file.repository} | ||
| repoName={file.repo} |
There was a problem hiding this comment.
Use a repo-qualified key for file rows.
Line [61] uses file.fileName as the React key. With cross-repo results, identical paths can collide and cause incorrect row reuse.
Suggested fix
- <FileListItem
- key={file.fileName}
+ <FileListItem
+ key={`${file.repo}:${file.fileName}`}
path={file.fileName}
repoName={file.repo}
/>📝 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.
| {part.output.metadata.files.map((file) => { | |
| return ( | |
| <FileListItem | |
| key={file.fileName} | |
| path={file.fileName} | |
| repoName={file.repository} | |
| repoName={file.repo} | |
| {part.output.metadata.files.map((file) => { | |
| return ( | |
| <FileListItem | |
| key={`${file.repo}:${file.fileName}`} | |
| path={file.fileName} | |
| repoName={file.repo} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/web/src/features/chat/components/chatThread/tools/grepToolComponent.tsx`
around lines 58 - 63, The file list map uses file.fileName as the React key
which can collide across repos; update the mapping in part.output.metadata.files
to use a repo-qualified key (e.g., combine file.repo and file.fileName) for the
FileListItem key prop so each row is uniquely identified across
repositories—modify the code that renders <FileListItem key={...} ...> to use a
composite key like `${file.repo}-${file.fileName}` (or another unique repo+path
identifier) and keep path={file.fileName} and repoName={file.repo} unchanged.
| const quotedPattern = `"${pattern.replace(/"/g, '\\"')}"`; | ||
| let query = quotedPattern; | ||
|
|
||
| if (path) { | ||
| query += ` file:${escapeStringRegexp(path)}`; | ||
| } | ||
|
|
||
| if (include) { | ||
| query += ` file:${globToFileRegexp(include)}`; | ||
| } | ||
|
|
||
| if (repo) { | ||
| query += ` repo:${escapeStringRegexp(repo)}`; | ||
| } | ||
|
|
||
| if (ref) { | ||
| query += ` (rev:${ref})`; | ||
| } |
There was a problem hiding this comment.
Harden query construction for pattern and ref.
Line [75] escapes quotes but not backslashes, and Line [91] interpolates ref unescaped. This allows crafted input to alter query semantics.
Suggested fix
- const quotedPattern = `"${pattern.replace(/"/g, '\\"')}"`;
+ const quotedPattern = JSON.stringify(pattern);
let query = quotedPattern;
...
- if (ref) {
- query += ` (rev:${ref})`;
- }
+ if (ref) {
+ query += ` (rev:${escapeStringRegexp(ref)})`;
+ }📝 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.
| const quotedPattern = `"${pattern.replace(/"/g, '\\"')}"`; | |
| let query = quotedPattern; | |
| if (path) { | |
| query += ` file:${escapeStringRegexp(path)}`; | |
| } | |
| if (include) { | |
| query += ` file:${globToFileRegexp(include)}`; | |
| } | |
| if (repo) { | |
| query += ` repo:${escapeStringRegexp(repo)}`; | |
| } | |
| if (ref) { | |
| query += ` (rev:${ref})`; | |
| } | |
| const quotedPattern = JSON.stringify(pattern); | |
| let query = quotedPattern; | |
| if (path) { | |
| query += ` file:${escapeStringRegexp(path)}`; | |
| } | |
| if (include) { | |
| query += ` file:${globToFileRegexp(include)}`; | |
| } | |
| if (repo) { | |
| query += ` repo:${escapeStringRegexp(repo)}`; | |
| } | |
| if (ref) { | |
| query += ` (rev:${escapeStringRegexp(ref)})`; | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 75-75: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/tools/grep.ts` around lines 75 - 92, The query
construction is vulnerable because pattern is only escaping quotes (not
backslashes) when building quotedPattern and ref is interpolated unescaped into
the rev clause; update the logic that builds quotedPattern to first escape
backslashes and quotes (e.g., replace "\" with "\\\\" then "\"" with "\\\"")
before surrounding with quotes, and sanitize/ref-escape the ref value before
interpolating (use the existing escapeStringRegexp or equivalent safe-quoting
function) when constructing the `(rev:...)` fragment; keep use of
escapeStringRegexp for repo/path and globToFileRegexp for include as-is.
| function buildRipgrepCommand({ pattern, path, include }: SearchInput): string { | ||
| const parts = ['rg', `"${pattern.replace(/"/g, '\\"')}"`]; | ||
| if (path) parts.push(path); | ||
| if (include) parts.push(`--glob "${include}"`); |
There was a problem hiding this comment.
Escape backslashes in quoted pattern builders (currently fails for \ inputs).
Line 25 and Line 32 only escape double quotes. Backslash-containing patterns (e.g., your Windows-path example) are emitted incorrectly and can produce malformed rg/zoekt queries.
Proposed fix
+function escapeQuotedTerm(value: string): string {
+ return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
+}
+
function buildRipgrepCommand({ pattern, path, include }: SearchInput): string {
- const parts = ['rg', `"${pattern.replace(/"/g, '\\"')}"`];
+ const parts = ['rg', `"${escapeQuotedTerm(pattern)}"`];
if (path) parts.push(path);
if (include) parts.push(`--glob "${include}"`);
return parts.join(' ');
}
function buildZoektQuery({ pattern, path, include }: SearchInput): string {
- const parts: string[] = [`"${pattern.replace(/"/g, '\\"')}"`];
+ const parts: string[] = [`"${escapeQuotedTerm(pattern)}"`];Also applies to: 31-33
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 25-25: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/tools/globToRegexpPlayground.ts` around lines 24 - 27, The
pattern quoting currently only escapes double quotes which breaks when the
pattern contains backslashes; update buildRipgrepCommand's pattern handling to
first escape backslashes (replace \ with \\) and then escape double quotes
(replace " with \\"), e.g. transform pattern via
pattern.replace(/\\/g,'\\\\').replace(/"/g,'\\"') before wrapping in quotes;
apply the same two-step escaping to the other quoted-pattern builders in this
file (the builders around lines 31-33) so backslashes and quotes are both
handled.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/web/src/features/chat/utils.ts (1)
341-343:includes(ANSWER_TAG)is too permissive and can cause premature answer detection.At Line 341, matching anywhere in text can produce false positives (e.g., literal tag in content), which may incorrectly set
answerPartduring streaming.Proposed fix
export const getAnswerPartFromAssistantMessage = (message: SBChatMessage, isStreaming: boolean): TextUIPart | undefined => { const lastTextPart = message.parts .findLast((part) => part.type === 'text') - if (lastTextPart?.text.includes(ANSWER_TAG)) { - return lastTextPart; + if (lastTextPart) { + const tagIndex = lastTextPart.text.indexOf(ANSWER_TAG); + if (tagIndex >= 0 && lastTextPart.text.slice(0, tagIndex).trim().length === 0) { + return lastTextPart; + } } // If the agent did not include the answer tag, then fallback to using the last text part.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/chat/utils.ts` around lines 341 - 343, The check using lastTextPart?.text.includes(ANSWER_TAG) is too permissive and should detect the tag only when it actually marks the start of an answer; update the condition in the logic that reads lastTextPart (referencing lastTextPart and ANSWER_TAG) to require the tag at the beginning of the text (for example by testing trimmed start or using a regex like ^ANSWER_TAG with word-boundary) instead of includes, so only texts that start with the answer tag are treated as answerPart.CHANGELOG.md (1)
11-11: Consider removing the comma before "and" for consistency.For a two-item list, the comma before "and" is unnecessary and inconsistent with standard changelog conventions. Consider either:
Added find_symbol_definitions and find_symbol_references tools to the MCP server.Added find_symbol_definitions & find_symbol_references tools to the MCP server.(following the pattern from line 145)✏️ Suggested improvement
-- Added `find_symbol_definitions`, and `find_symbol_references` tools to the MCP server. [`#1014`](https://github.com/sourcebot-dev/sourcebot/pull/1014) +- Added `find_symbol_definitions` and `find_symbol_references` tools to the MCP server. [`#1014`](https://github.com/sourcebot-dev/sourcebot/pull/1014)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 11, Update the sentence in CHANGELOG.md that mentions find_symbol_definitions and find_symbol_references to remove the comma before "and" so it reads either "Added find_symbol_definitions and find_symbol_references tools to the MCP server." (or use the ampersand alternative to match line 145); target the line containing the identifiers find_symbol_definitions and find_symbol_references and adjust the punctuation accordingly for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CHANGELOG.md`:
- Line 11: Update the sentence in CHANGELOG.md that mentions
find_symbol_definitions and find_symbol_references to remove the comma before
"and" so it reads either "Added find_symbol_definitions and
find_symbol_references tools to the MCP server." (or use the ampersand
alternative to match line 145); target the line containing the identifiers
find_symbol_definitions and find_symbol_references and adjust the punctuation
accordingly for consistency.
In `@packages/web/src/features/chat/utils.ts`:
- Around line 341-343: The check using lastTextPart?.text.includes(ANSWER_TAG)
is too permissive and should detect the tag only when it actually marks the
start of an answer; update the condition in the logic that reads lastTextPart
(referencing lastTextPart and ANSWER_TAG) to require the tag at the beginning of
the text (for example by testing trimmed start or using a regex like ^ANSWER_TAG
with word-boundary) instead of includes, so only texts that start with the
answer tag are treated as answerPart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60d39693-8cde-422a-9981-3df24dcc6234
📒 Files selected for processing (2)
CHANGELOG.mdpackages/web/src/features/chat/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/features/chat/components/chatThread/tools/shared.tsx (1)
103-120:⚠️ Potential issue | 🟠 MajorDon't make the whole header a pseudo-button around another button.
The new copy control is now nested inside a focusable, clickable
div. That container still lacks button semantics/expanded state, does not handle Space, and keyboard events from the copy button can still bleed into the expand handler. Please split expansion onto its own button (or equivalent) instead of wrapping both controls in one interactive region.Also applies to: 137-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/chat/components/chatThread/tools/shared.tsx` around lines 103 - 120, The header currently makes the entire div (tabIndex, onClick, onKeyDown) act as an interactive pseudo-button and contains an inner copy control; split that behavior by replacing the clickable/focusable div with a non-interactive wrapper and move the expansion behavior to a dedicated <button> element that calls onExpand(!isExpanded), supports both Enter and Space, and sets aria-expanded={isExpanded}; ensure the copy control remains outside that button (or stopsPropagation on its keyboard/click handlers) so its events don't bubble into the expand handler; apply the same change pattern to the other occurrence around lines 137-143 (same className/isLoading/onExpand/isExpanded handlers).
♻️ Duplicate comments (2)
packages/web/src/features/chat/components/chatThread/tools/shared.tsx (1)
92-100:⚠️ Potential issue | 🟡 MinorWait for the clipboard write before reporting success.
navigator.clipboard.writeText()is async and can reject or be unavailable, but this handler always returnstrueimmediately. That makes the shared copy UI claim success on failures and can surface unhandled rejections across every tool card.If
CopyIconButtonis still synchronous, either widen it to() => boolean | Promise<boolean>and await it there, or at least catch failures in this shared handler.#!/bin/bash # Verify the current copy-button contract and all clipboard call sites. fd 'copyIconButton\.tsx$' packages/web/src -x sed -n '1,220p' {} rg -n -C2 'navigator\.clipboard\.writeText|onCopy\??\s*:' packages/web/src/features/chat/components/chatThread/tools packages/web/src/app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/chat/components/chatThread/tools/shared.tsx` around lines 92 - 100, The onCopy handler currently calls navigator.clipboard.writeText(...) but returns true immediately, so failures/unhandled rejections and false positives occur; update the handler to await navigator.clipboard.writeText(...) and catch errors, returning true only on success and false on failure (or rethrow/log the error), and if CopyIconButton enforces a synchronous signature change its type to allow boolean | Promise<boolean> (or adapt its call sites) so awaiting is supported; reference symbols: onCopy, navigator.clipboard.writeText, and CopyIconButton.packages/web/src/features/tools/grep.ts (1)
68-85:⚠️ Potential issue | 🟠 MajorEscape
patternandrefbefore interpolating them into the query.
pattern.replace(/"/g, '\\"')still leaves backslashes unescaped, andrefis inserted verbatim intorev:. Crafted input can still alter query parsing or defeat the repo/path scoping you build around it.🔒 Proposed fix
- const quotedPattern = `"${pattern.replace(/"/g, '\\"')}"`; + const quotedPattern = JSON.stringify(pattern); let query = quotedPattern; @@ - if (ref) { - query += ` (rev:${ref})`; - } + if (ref) { + query += ` (rev:${escapeStringRegexp(ref)})`; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/tools/grep.ts` around lines 68 - 85, The pattern and ref are interpolated into the search query without fully escaping backslashes or other regex-special characters; update the code that builds quotedPattern and the rev insertion in the query so you first escape the pattern and the ref using the existing escapeStringRegexp helper (or equivalent escaping that also escapes backslashes) before doing the double-quote escape and interpolation. Specifically, apply escapeStringRegexp to pattern (then replace any " with \" and wrap in quotes when creating quotedPattern) and apply escapeStringRegexp to ref when adding `(rev:${ref})`, leaving other uses of escapeStringRegexp/globToFileRegexp (for path/include) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/web/src/features/chat/components/chatThread/tools/grepToolComponent.tsx`:
- Around line 51-56: The FileListItem is being passed the filename basename
(file.name) instead of the repo-relative path, breaking navigation for nested
matches; in grepToolComponent.tsx change the prop to pass file.path to
FileListItem (key stays file.path) so FileListItem builds correct browse URLs,
and if you want grep results to open a non-HEAD ref also thread file.revision
through the call chain (update
packages/web/src/features/chat/components/chatThread/tools/shared.tsx and any
FileListItem usage to accept and use file.revision instead of hardcoding
"HEAD").
In `@packages/web/src/features/tools/grep.ts`:
- Around line 40-44: The public tool schema for `limit` currently allows
negatives and huge numbers; update the `limit` schema entry (the zod chain
starting with `limit: z.number()...`) to enforce an integer between sensible
bounds—e.g. replace the current chain with
`z.number().int().min(1).max(<MAX_SEARCH_LIMIT>).default(DEFAULT_SEARCH_LIMIT).describe(...).optional()`
where `<MAX_SEARCH_LIMIT>` is either an existing constant or a newly defined
sane cap (e.g. 1000); this ensures `limit` is a positive integer and prevents
excessive work before `search()` runs.
---
Outside diff comments:
In `@packages/web/src/features/chat/components/chatThread/tools/shared.tsx`:
- Around line 103-120: The header currently makes the entire div (tabIndex,
onClick, onKeyDown) act as an interactive pseudo-button and contains an inner
copy control; split that behavior by replacing the clickable/focusable div with
a non-interactive wrapper and move the expansion behavior to a dedicated
<button> element that calls onExpand(!isExpanded), supports both Enter and
Space, and sets aria-expanded={isExpanded}; ensure the copy control remains
outside that button (or stopsPropagation on its keyboard/click handlers) so its
events don't bubble into the expand handler; apply the same change pattern to
the other occurrence around lines 137-143 (same
className/isLoading/onExpand/isExpanded handlers).
---
Duplicate comments:
In `@packages/web/src/features/chat/components/chatThread/tools/shared.tsx`:
- Around line 92-100: The onCopy handler currently calls
navigator.clipboard.writeText(...) but returns true immediately, so
failures/unhandled rejections and false positives occur; update the handler to
await navigator.clipboard.writeText(...) and catch errors, returning true only
on success and false on failure (or rethrow/log the error), and if
CopyIconButton enforces a synchronous signature change its type to allow boolean
| Promise<boolean> (or adapt its call sites) so awaiting is supported; reference
symbols: onCopy, navigator.clipboard.writeText, and CopyIconButton.
In `@packages/web/src/features/tools/grep.ts`:
- Around line 68-85: The pattern and ref are interpolated into the search query
without fully escaping backslashes or other regex-special characters; update the
code that builds quotedPattern and the rev insertion in the query so you first
escape the pattern and the ref using the existing escapeStringRegexp helper (or
equivalent escaping that also escapes backslashes) before doing the double-quote
escape and interpolation. Specifically, apply escapeStringRegexp to pattern
(then replace any " with \" and wrap in quotes when creating quotedPattern) and
apply escapeStringRegexp to ref when adding `(rev:${ref})`, leaving other uses
of escapeStringRegexp/globToFileRegexp (for path/include) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef5fab93-9e3b-4f4c-b013-a8ca84d381b1
📒 Files selected for processing (12)
packages/web/src/features/chat/agent.tspackages/web/src/features/chat/components/chatThread/tools/findSymbolDefinitionsToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/findSymbolReferencesToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/grepToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listCommitsToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listReposToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/listTreeToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/readFileToolComponent.tsxpackages/web/src/features/chat/components/chatThread/tools/shared.tsxpackages/web/src/features/tools/grep.tspackages/web/src/features/tools/grep.txtpackages/web/src/features/tools/readFile.ts
✅ Files skipped from review due to trivial changes (1)
- packages/web/src/features/tools/grep.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/src/features/tools/readFile.ts
- packages/web/src/features/chat/components/chatThread/tools/listTreeToolComponent.tsx
| {part.output.metadata.files.map((file) => { | ||
| return ( | ||
| <FileListItem | ||
| key={file.path} | ||
| path={file.name} | ||
| repoName={file.repo} |
There was a problem hiding this comment.
Use the repo-relative path here, not the basename.
GrepMetadata.files already separates path and name, but FileListItem uses its path prop to build the browse URL. Passing file.name breaks navigation for nested matches, and this card still cannot honor grep results from non-HEAD refs until revision is threaded through FileListItem.
🐛 Proposed fix
- <FileListItem
- key={file.path}
- path={file.name}
- repoName={file.repo}
- />
+ <FileListItem
+ key={file.path}
+ path={file.path}
+ repoName={file.repo}
+ />This local fix restores correct paths. If grep should open the matched ref, packages/web/src/features/chat/components/chatThread/tools/shared.tsx also needs to accept and use file.revision instead of hardcoding HEAD.
📝 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.
| {part.output.metadata.files.map((file) => { | |
| return ( | |
| <FileListItem | |
| key={file.path} | |
| path={file.name} | |
| repoName={file.repo} | |
| {part.output.metadata.files.map((file) => { | |
| return ( | |
| <FileListItem | |
| key={file.path} | |
| path={file.path} | |
| repoName={file.repo} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/web/src/features/chat/components/chatThread/tools/grepToolComponent.tsx`
around lines 51 - 56, The FileListItem is being passed the filename basename
(file.name) instead of the repo-relative path, breaking navigation for nested
matches; in grepToolComponent.tsx change the prop to pass file.path to
FileListItem (key stays file.path) so FileListItem builds correct browse URLs,
and if you want grep results to open a non-HEAD ref also thread file.revision
through the call chain (update
packages/web/src/features/chat/components/chatThread/tools/shared.tsx and any
FileListItem usage to accept and use file.revision instead of hardcoding
"HEAD").
| limit: z | ||
| .number() | ||
| .default(DEFAULT_SEARCH_LIMIT) | ||
| .describe(`The maximum number of matches to return (default: ${DEFAULT_SEARCH_LIMIT})`) | ||
| .optional(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clamp limit in the public tool schema.
limit is now exposed through both the agent and MCP, but the schema still accepts negative and arbitrarily large values. A bad model output or client request can turn this into backend validation errors or very expensive searches. Constrain it to a sane positive integer at the Zod boundary instead of relying on search() to clean it up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/tools/grep.ts` around lines 40 - 44, The public
tool schema for `limit` currently allows negatives and huge numbers; update the
`limit` schema entry (the zod chain starting with `limit: z.number()...`) to
enforce an integer between sensible bounds—e.g. replace the current chain with
`z.number().int().min(1).max(<MAX_SEARCH_LIMIT>).default(DEFAULT_SEARCH_LIMIT).describe(...).optional()`
where `<MAX_SEARCH_LIMIT>` is either an existing constant or a newly defined
sane cap (e.g. 1000); this ensures `limit` is a positive integer and prevents
excessive work before `search()` runs.
1 participant
Summary
ToolDefinition<TName, TShape, TMetadata>abstraction infeatures/tools/with a unifiedexecute(input, context)signature andToolResult<TMetadata>return shaperead_file,list_commits,list_repos,search_code,find_symbol_references,find_symbol_definitions,list_tree) are now defined once and registered with both the Vercel AI agent and the MCP server viatoVercelAIToolandregisterMcpTooladapterssource: 'agent'orsource: 'mcp') into every tool executionisReadOnlyandisIdempotenthints are declared on each tool definition and forwarded to MCPannotationsviaregisterMcpTooltoolNamesconstant eliminated in favour ofxDefinition.namelist_treetool to the Vercel AI agent with a UI component in the details cardrepository→repo);webUrladded tosearch_codeoutputlogger.tsfor tools; debug logging added to all tools using consistent snake_case nameslist_reposdiscovery,webUrllinking)ToolHeaderso all tool components get it for freeTODO:
search_codetoolTest plan
isReadOnly/isIdempotentannotations appear in MCP tool listings (e.g. Cursor Ask mode)🤖 Generated with Claude Code
Summary by CodeRabbit