refactor(web): unify tool definitions between ask agent and MCP server#1014
Open
brendan-kellam wants to merge 16 commits intomainfrom
Open
refactor(web): unify tool definitions between ask agent and MCP server#1014brendan-kellam wants to merge 16 commits intomainfrom
brendan-kellam wants to merge 16 commits intomainfrom
Conversation
Contributor
WalkthroughConsolidates scattered tool definitions into a centralized tools surface, adds adapters for Vercel AI and MCP integration, introduces multiple read-only tools (search_code, read_file, list_tree, list_repos, list_commits, find_symbol_*), updates chat UI to consume metadata-shaped outputs, and wires MCP server registrations declaratively. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (browser)
participant Agent as Next Agent
participant Vercel as Vercel AI Runtime
participant Tool as ToolDefinition
participant Service as External Service
rect rgba(100,149,237,0.5)
Client->>Agent: User invokes assistant/tool action
Agent->>Vercel: Stream prompt + tool call (via toVercelAITool)
Vercel->>Tool: Execute tool (input parsed by adapter)
Tool->>Service: Call backend API (search/read/list)
Service-->>Tool: Return data/metadata
Tool-->>Vercel: Return { output: string, metadata }
Vercel-->>Agent: Tool output streamed
Agent-->>Client: Render message using output.metadata.* (UI components)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 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 |
Contributor
License Audit❌ Status: FAIL
Fail Reasons
Unresolved Packages
Weak Copyleft Packages (informational)
Resolved Packages (16)
|
This comment has been minimized.
This comment has been minimized.
Contributor
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 |
Contributor
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.
Comment on lines
+66
to
+73
| turbopack: { | ||
| rules: { | ||
| '*.txt': { | ||
| loaders: ['raw-loader'], | ||
| as: '*.js', | ||
| }, | ||
| }, | ||
| }, |
Contributor
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.
Suggested change
| 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.
Comment on lines
+55
to
+60
| {part.output.metadata.files.map((file) => { | ||
| return ( | ||
| <FileListItem | ||
| key={file.fileName} | ||
| path={file.fileName} | ||
| repoName={file.repository} | ||
| repoName={file.repo} |
Contributor
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.
Suggested change
| {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.
Comment on lines
+26
to
+28
| const onCopy = part.state === 'output-available' && !isServiceError(part.output) | ||
| ? () => { navigator.clipboard.writeText(part.output.output); return true; } | ||
| : undefined; |
Contributor
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.
Suggested change
| 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.
Comment on lines
+26
to
+28
| const onCopy = part.state === 'output-available' && !isServiceError(part.output) | ||
| ? () => { navigator.clipboard.writeText(part.output.output); return true; } | ||
| : undefined; |
Contributor
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.
Comment on lines
+63
to
+65
| const queue: Array<{ path: string; depth: number }> = [{ path: normalizedPath, depth: 0 }]; | ||
| const queuedPaths = new Set<string>([normalizedPath]); | ||
| const seenEntries = new Set<string>(); |
Contributor
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.
Comment on lines
+77
to
+81
| const treeResult = await getTree({ | ||
| repoName: repo, | ||
| revisionName: ref, | ||
| paths: currentLevelPaths.filter(Boolean), | ||
| }, { source: context.source }); |
Contributor
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.
Comment on lines
+57
to
+106
| 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, | ||
| }; |
Contributor
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.
Comment on lines
+44
to
+48
| limit: z | ||
| .number() | ||
| .default(DEFAULT_SEARCH_LIMIT) | ||
| .describe(`Maximum number of matches to return (default: ${DEFAULT_SEARCH_LIMIT})`) | ||
| .optional(), |
Contributor
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.
Suggested change
| 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.
Comment on lines
+83
to
+97
| 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})`; | ||
| } |
Contributor
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().
Contributor
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
Comment on lines
+79
to
+81
| annotations: { | ||
| readOnlyHint: true, | ||
| } |
Contributor
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.
Suggested change
| 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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
New Features
Improvements