feat: [ENG-2124] Improve MCP tool descriptions#423
Conversation
|
Claude finished @bao-byterover's task in 2m 8s —— View job PR Review: feat: [ENG-2124] Improve MCP tool descriptionsOverall Score: 4/5 — Clean, well-scoped refactor with genuine value. Single source of truth eliminates drift, and the capability-first description is a real improvement for LLM consumers. Two minor concerns worth addressing before merge. SummaryThe deduplication approach is sound: replacing two copies of an inline Issuessuggestion · The new description tells the LLM: "If your runtime DOES expose an open workspace/project root to you (e.g., Cursor, Cline, Zed, Claude Code): you can OMIT this field — the MCP server was launched from that same project." This claim is only valid in project mode. If Concrete failure scenario:
Consider a blended framing: lead with the capability-first check (keeps it LLM-actionable), but add a fallback clause for when things go wrong: // suggested rewrite (lines 7–16):
'When to provide:\n' +
'- If your runtime does NOT expose a workspace/project root ' +
'(e.g., Claude Desktop, hosted MCP, global Windsurf): you MUST provide cwd. ' +
'Use the path the user mentions, or ASK the user for the absolute path if unknown.\n' +
'- If your runtime exposes a workspace root AND ByteRover answers correctly without cwd ' +
'(e.g., Cursor, Cline, Zed, Claude Code in project-mode MCP): you may OMIT this field.\n' +
'- If you omit cwd and get a "no project found" error, provide cwd explicitly.\n' +
'\n' +
'Never guess, never invent paths, never use relative paths.'The key change is adding the error-recovery escape hatch so the LLM has a recovery path rather than being stuck in a dead end. issue (non-blocking) · Per CLAUDE.md TDD requirements, new implementation files need tests written first. These would be trivial to add and would lock in the contract: // suggested test additions in a new test/unit/infra/mcp/tools/shared-schema.test.ts
import { cwdField, CWD_DESCRIPTION } from '../../../../../src/server/infra/mcp/tools/shared-schema.js'
import { expect } from 'chai'
describe('shared-schema', () => {
it('cwdField accepts undefined (optional)', () => {
expect(cwdField.safeParse(undefined).success).to.be.true
})
it('cwdField accepts an absolute path string', () => {
expect(cwdField.safeParse('/Users/me/project').success).to.be.true
})
it('cwdField description matches CWD_DESCRIPTION', () => {
expect(cwdField.description).to.equal(CWD_DESCRIPTION)
})
it('CWD_DESCRIPTION contains the "Never guess" rule', () => {
expect(CWD_DESCRIPTION).to.include('Never guess')
})
})Praisepraise ·
praise · The praise · Re-exporting Checklist
|
|
Claude finished @bao-byterover's task in 1m 24s —— View job PR Review: feat: [ENG-2124] Improve MCP tool descriptionsOverall Score: 4/5 — The primary concern from the previous review is properly resolved. The blended description framing + error-recovery clause is the right approach. One minor gap remains in the new test file. What Changed Since Last ReviewThe fix commit (
Assessment of Previous Issuessuggestion (resolved) · The rework is correct. The description now leads with the "no runtime context" case (force MUST provide This is exactly what was missing. The global-mode failure scenario is now recoverable. suggestion (partially addressed) · The test file was added, which addresses the gap. However, two of the four suggested assertions are missing: // Present ✓
it('accepts undefined (optional)', ...)
it('accepts an absolute path string', ...)
// Missing ✗
it('cwdField description matches CWD_DESCRIPTION', () => {
expect(cwdField.description).to.equal(CWD_DESCRIPTION)
})
it('CWD_DESCRIPTION contains the "Never guess" rule', () => {
expect(CWD_DESCRIPTION).to.include('Never guess')
})Note that These are non-blocking — the Zod shape tests serve as a regression net — but the description assertions are the ones that would catch a future accidental Unchanged Files — No Issues
Praisepraise · Leading with the "does NOT expose context → MUST provide" case is the right instruction order — it handles the more dangerous scenario first and gives the LLM an explicit recovery action ("ASK the user") rather than leaving it stuck. praise · The retry-on-error clause is exactly what was needed. The LLM now has a deterministic recovery path instead of a dead end. Checklist
|
|
Claude finished @bao-byterover's task in 1m 10s —— View job PR Review: feat: [ENG-2124] Improve MCP tool descriptionsOverall Score: 5/5 — All previous review concerns fully resolved. Clean, complete refactor with no outstanding issues. What Changed Since Last Review (commit
|
2 participants
Summary
brv-curateandbrv-queryframed thecwdfield around server runtime state (project-mode vs global-mode) — information LLMs cannot detect. Descriptions were also duplicated between the two tools, risking drift.cwd, leading to either regression (omitting it inappropriately) or guessed/invented paths. No server-level onboarding existed before the first tool call.src/server/infra/mcp/tools/shared-schema.tsexportingCWD_DESCRIPTIONandcwdField— a single source of truth for thecwdZod field.CWD_DESCRIPTIONto be capability-first: instructs the LLM based on whether its own runtime exposes a workspace/project root (checkable), not the server's launch mode (not checkable).brv-curate-tool.tsandbrv-query-tool.tsto usecwdFieldinstead of inline duplicated schemas.instructionsstring on theMcpServerconstructor as a pointer to the per-toolcwddescription (avoids drift, minimal tokens).cwdremainsz.string().optional().context,files,folder,queryfields untouched.resolveClientCwd, orresolveMcpTaskContext.Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
test/unit/infra/mcp/tools/brv-curate-tool.test.tsused as regression net (30 tests coveringBrvCurateInputSchemashape + handler paths).cwdas optional absolute path (validation unchanged).BrvCurateInputSchema.shape.cwdandBrvQueryInputSchema.shape.cwdboth reference the sharedcwdField.User-visible changes
brv-curate/brv-queryMCP tools (cwdfield text, visible to coding agents via MCP schema introspection).instructionsstring exposed to MCP clients oninitializeresponse.Evidence
Checklist
npm test) — existing tests pass (description-only refactor, no new tests needed per CLAUDE.md TDD note on zero-behavior-change refactors)npm run lint) — 0 errorsnpm run typecheck)npm run build)mainRisks and mitigations
cwddescriptions.instructionsinto their LLM system prompt (behavior varies by client).cwddescription carries the full rule and is always injected with the tool schema;instructionsis a short pointer, so clients that ignore it lose nothing substantive.