Repository navigation
CTX7-3522: Docs feedback score and required comment, with a feedback request in the server instructions - #3617
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
fahreddinozcan
left a comment
fahreddinozcan
left a comment
There was a problem hiding this comment.
Code quality review. I found no correctness bugs, and the behavior matches the description. I ask for one structural change before merge: remove the toolClientContext wrapper. The other comments are smaller.
The app side review is on upstash/context7app#1599.
| const server = mcpInstrumentation | ||
| ? mcpInstrumentation.createServer(serverInfo, serverOptions, mcpContext) | ||
| : new McpServer(serverInfo, serverOptions); | ||
| const toolClientContext = (toolCtx: ServerContext): ClientContext => ({ |
There was a problem hiding this comment.
This wrapper is not necessary. ClientContext is already built in exactly two places: the HTTP context object (const context: ClientContext = {...} in the request handler) and the stdio fallback in getClientContext.
Suggestion: set researchPreview there.
- HTTP: read it from the same
research_previewquery parameter (isResearchPreviewValue). - stdio: use a module-level flag, like
stdioSessionId.
Then this wrapper, the 3 changed call sites, the changed registerResearchPreviewTools signature and the ...(x ? { researchPreview: true } : {}) spread all go away.
There was a problem hiding this comment.
Fixed in fa3d031. The HTTP context reads research_preview from the query with isResearchPreviewValue, and stdio uses a module-level stdioResearchPreview next to stdioSessionId. The wrapper is gone, the call sites use getClientContext again, and registerResearchPreviewTools has its old signature.
| import type { ClientContext, DocsFeedback } from "./types.js"; | ||
|
|
||
| /** Added to the server instructions with research preview on, so agents remember the feedback tool. */ | ||
| export const DOCS_FEEDBACK_INSTRUCTIONS = `After you answer a question with documentation from 'query-docs', call '${SUBMIT_DOCS_FEEDBACK_TOOL}' once for that library with a score from 0 to 5 and a short comment on the docs. The user turned on this research preview to collect this feedback.`; |
There was a problem hiding this comment.
'query-docs' is a literal string here, but QUERY_DOCS_TOOL exists and the next tool name already uses its constant. Use ${QUERY_DOCS_TOOL}.
There was a problem hiding this comment.
Fixed in fa3d031. It uses ${QUERY_DOCS_TOOL} now.
| ), | ||
| author: z | ||
| .enum(["agent", "user"]) | ||
| .min(1) |
There was a problem hiding this comment.
This does not match the app contract. The app trims the text, requires at least 1 character after the trim, and rejects more than 1,000 characters. Here " " passes and there is no maximum, so the agent gets a 400 from the app and not a schema error. Suggestion: .trim().min(1).max(1000).
There was a problem hiding this comment.
Fixed in fa3d031. The comment is .trim().min(1).max(1000) now, the same as the app.
| libraryId: string; | ||
| query?: string; | ||
| helpful: boolean; | ||
| score: number; |
There was a problem hiding this comment.
DocsFeedback copies the zod input schema by hand (the issue enum is in both places). Suggestion: move the schema to a constant and use export type DocsFeedback = z.infer<typeof docsFeedbackSchema>.
There was a problem hiding this comment.
Fixed in fa3d031. The schema is the docsFeedbackSchema constant, and DocsFeedback is z.infer<typeof docsFeedbackSchema>. I removed the copy from types.ts.
fahreddinozcan
left a comment
fahreddinozcan
left a comment
There was a problem hiding this comment.
All review comments are addressed, and CI passes. Deploy after upstash/context7app#1599, as the description says. Thanks.
Summary
submit-docs-feedbacktakesscore(whole number 0 to 5) in place ofhelpful.commentis required.authoris removed. The tool asks forissueonly when the score is below 5.query-docs, callsubmit-docs-feedbackonce with a score and a short comment.X-Context7-Research-Preview: 1, so the app can count how many people use the preview (app Refresh request for /websites/developers_facebook_marketing-api #1599).Deploy order
Deploy the app PR first: https://github.com/upstash/context7app/pull/1599. The route must accept
scorebefore this MCP sends it.Benchmark
10 tasks taken from real
query-docsqueries, each ending with "use context7". The MCP was the local build with research preview on, behind a local proxy. The proxy sent docs queries to production and kept all feedback local.Feedback calls per 10 runs:
setParamspages disagree, the Tailwind v4 install snippet comes from the alpha blog (@next), and Next.js has no reverse-proxy redirect guide.Part of CTX7-3522