refactor: clean up debug logs, deduplicate rate-limit handling and output logic#6
Open
quotentiroler wants to merge 13 commits into
Open
refactor: clean up debug logs, deduplicate rate-limit handling and output logic#6quotentiroler wants to merge 13 commits into
quotentiroler wants to merge 13 commits into
Conversation
…tput logic
- Remove 43 console.error('[DBG]') debug statements
- Extract isRateLimitError() and rateLimitSummary() to src/utils/errors.ts
- Extract shared output path/write logic to src/utils/output.ts
- Simplify callGeminiProConfigurable() by removing redundant try/catch
- Clean up truncateLearnings() module-level coupling
- Remove unreachable comment in processGeminiResponse()
- Clean .gitignore: deduplicate entries, fix typos, organize sections
- Improve JSON extraction with brace-balanced parser (src/utils/json.ts)
- Simplify SERP query prompt template (src/prompt.ts)
- Redirect progress output to stderr for MCP stdio compliance
Reviewer's GuideRefactors deep research, provider, and MCP CLI/server code to centralize error/output handling, improve JSON extraction and prompt design, surface rate-limit errors, and ensure MCP-compatible logging and file output, while cleaning up debug noise and gitignore entries. Sequence diagram for MCP deepResearch_run tool with centralized output and error handlingsequenceDiagram
actor User
participant Client as MCP_client
participant MCP as MCPServerModule
participant DR as DeepResearchModule
participant Prov as ProvidersModule
participant UE as UtilsErrors
participant UO as UtilsOutput
participant FS as FileSystem
User->>Client: Invoke tool deepResearch_run
Client->>MCP: JSON-RPC request
MCP->>MCP: Build cacheKey
alt cached result
MCP-->>Client: Cached MCPResearchResult
else cache miss
MCP->>DR: deepResearch(query, depth, breadth, existingLearnings, onProgress)
DR->>Prov: callGeminiProConfigurable for SERP and research calls
Prov->>Prov: generateContentInternal
Prov->>UE: isRateLimitError(apiErr)? (on failure)
alt API rate limited
UE-->>Prov: true
Prov->>UE: rateLimitSummary(apiErr)
Prov->>Prov: log rate-limit details
Prov-->>DR: throw apiErr
DR->>UE: isRateLimitError(err)
DR->>UE: rateLimitSummary(err)
DR-->>MCP: ResearchResult with errors and no learnings
else API ok
Prov-->>DR: Gemini responses
DR->>DR: processGeminiResponse, generateOutline, writeReportFromOutline, generateSummary, generateTitle
DR-->>MCP: ResearchResult (learnings, visitedUrls, errors?)
end
alt no learnings and errors present
MCP->>MCP: Build error-only MCPResearchResult
MCP-->>Client: Error summary + metadata.errors
else learnings present
MCP->>DR: writeFinalReport(prompt, learnings, visitedUrls)
DR-->>MCP: report markdown
MCP->>UO: resolveOutputDir(outputDir, defaultPath)
UO-->>MCP: resolvedDir
MCP->>UO: writeResearchOutput(resolvedDir, query, report, learnings, visitedUrls, depth, breadth)
UO->>FS: mkdir(outputDir), writeFile(report, learnings.json)
FS-->>UO: ok
UO-->>MCP: { reportPath, learningsPath }
MCP->>MCP: Build inlineText (truncate if >4000 chars)
MCP-->>Client: MCPResearchResult with content, metadata.stats, metadata.outputPath, metadata.errors
end
end
Client-->>User: Shows report snippet and output file path
Class diagram for deep research, provider, and new utils structureclassDiagram
class DeepResearchModule {
+generateSerpQueries(query string, numQueries number, learnings string[], researchGoal string, initialQuery string, depth number, breadth number) Promise~SerpQuery[]~
+truncateLearnings(learnings string[], maxChars number) string[]
+generateOutline(prompt string, learnings string[]) Promise~string~
+writeReportFromOutline(outline string, learnings string[]) Promise~string~
+generateSummary(learnings string[]) Promise~string~
+generateTitle(prompt string, learnings string[]) Promise~string~
+deepResearch(query string, depth number, breadth number, existingLearnings string[], onProgress ResearchProgressCallback) Promise~ResearchResult~
+processGeminiResponse(geminiResponseText string) Promise~ProcessedGeminiResponse~
}
class ResearchResult {
+content string
+sources string[]
+methodology string
+limitations string
+citations string[]
+learnings string[]
+visitedUrls string[]
+firecrawlResults SearchResponse
+analysis string
+errors string[]
}
class ProcessResult {
+analysis string
+content string
+methodology string
+limitations string
+citations string[]
+learnings string[]
+visitedUrls string[]
+firecrawlResults SearchResponse
+errors string[]
}
class UtilsErrors {
+isRateLimitError(err unknown) boolean
+rateLimitSummary(err unknown, fallback string) string
}
class UtilsOutput {
+buildOutputPaths(baseDir string, query string) OutputPaths
+resolveOutputDir(explicit string, fallback string) string
+writeResearchOutput(outputDir string, query string, report string, learnings string[], visitedUrls string[], depth number, breadth number) Promise~OutputPaths~
}
class OutputPaths {
+reportPath string
+learningsPath string
+timestamp string
}
class OutputManager {
+log(message string, data any) void
+updateProgress(progress ResearchProgress) void
+flushLogs() void
+saveResearchReport(content string) void
+static logCacheEviction(value unknown) void
}
class ProvidersModule {
+generateContentInternal(prompt ContentArg, extra GenExtra) Promise~GenerateWrapped~
+callGeminiProConfigurable(prompt string, opts GenExtra) Promise~string~
}
class GenerateWrapped {
+response GenerateWrappedResponse
}
class GenerateWrappedResponse {
+text() Promise~string~
}
class PromptModule {
+serpQueryPromptTemplate string
+learningPromptTemplate string
+generateGeminiPrompt(query string, researchGoal string, learnings string[]) string
}
class MCPServerModule {
+deepResearch_run(query string, depth number, breadth number, existingLearnings string[], goal string, outputDir string) Promise~MCPResearchResult~
}
class MCPResearchResult {
+content MCPContentPart[]
+metadata MCPMetadata
}
class MCPMetadata {
+learnings string[]
+visitedUrls string[]
+outputPath string
+stats MCPStats
+errors string[]
}
class MCPStats {
+totalLearnings number
+totalSources number
+reportLength number
+truncated boolean
}
class RunCliModule {
+runInteractiveResearch() Promise~void~
}
class JsonUtils {
+extractJsonFromText(text string) any
+safeParseJSON(text string, fallback any) any
+isValidJSON(text string) boolean
}
UtilsErrors <.. DeepResearchModule : uses
UtilsErrors <.. ProvidersModule : uses
UtilsOutput <.. MCPServerModule : uses
UtilsOutput <.. RunCliModule : uses
DeepResearchModule --> ResearchResult
DeepResearchModule --> ProcessResult
DeepResearchModule --> OutputManager
DeepResearchModule --> JsonUtils
DeepResearchModule --> PromptModule
DeepResearchModule --> ProvidersModule
ProvidersModule --> GenerateWrapped
GenerateWrapped --> GenerateWrappedResponse
MCPServerModule --> MCPResearchResult
MCPResearchResult --> MCPMetadata
MCPMetadata --> MCPStats
RunCliModule --> DeepResearchModule
RunCliModule --> UtilsOutput
PromptModule <.. DeepResearchModule : uses
JsonUtils <.. DeepResearchModule : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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:
WalkthroughAdds centralized rate‑limit detection and API‑key validation; strengthens JSON extraction and JSON‑only prompts; surfaces errors in research result types; introduces timestamped output helpers and writing; routes logs and eviction messages to stderr; and normalizes Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MCP as MCPServer
participant Validator as ApiKeyValidator
participant Research as DeepResearch
participant Gemini as GeminiAPI
participant Writer as OutputWriter
participant FS as Filesystem
Client->>MCP: deepResearch_run(query, outputDir?)
MCP->>Validator: validateApiKey()
Validator-->>MCP: tierInfo / error
MCP->>Research: runResearch(query, depth, breadth)
Research->>Gemini: generate SERP queries / generate content
alt rate-limit or error
Gemini-->>Research: rate-limit / error
Research-->>MCP: result with errors[]
else success
Gemini-->>Research: JSON/text responses
Research->>Writer: report, learnings, visitedUrls
Writer->>FS: write markdown + learnings.json
FS-->>Writer: reportPath, learningsPath
Writer-->>MCP: output paths
end
MCP-->>Client: MCPResearchResult (metadata: outputPath, errors, truncated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the codebase's maintainability and robustness by centralizing common functionalities and improving error handling. It streamlines the process of managing API rate limits, standardizes file output operations, and makes JSON extraction more reliable. These changes contribute to a cleaner, more efficient, and more compliant application architecture, particularly in how it interacts with external APIs and handles its own logging. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
rateLimitSummary, themodelcapture regexmodel: ([\w-]+)will truncate model IDs that contain dots (e.g.gemini-2.5-flash→gemini-2); consider broadening the character class (e.g.[\w.-]+) so logs/reporting include the full model name. - In
writeResearchOutput, the JSON sidecar setsgoal: opts.query, but the calling code already distinguishes betweenqueryand an optional researchgoal; consider threading the actual goal through the function signature so it can be persisted correctly instead of duplicating the query.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `rateLimitSummary`, the `model` capture regex `model: ([\w-]+)` will truncate model IDs that contain dots (e.g. `gemini-2.5-flash` → `gemini-2`); consider broadening the character class (e.g. `[\w.-]+`) so logs/reporting include the full model name.
- In `writeResearchOutput`, the JSON sidecar sets `goal: opts.query`, but the calling code already distinguishes between `query` and an optional research `goal`; consider threading the actual goal through the function signature so it can be persisted correctly instead of duplicating the query.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides an excellent and comprehensive refactoring, significantly improving code quality, robustness, and maintainability through centralized rate-limit error handling, output file logic, and more resilient JSON parsing from LLM responses. However, it introduces a critical security vulnerability by allowing arbitrary file writes via the new outputDir parameter in the MCP server. This parameter lacks validation and sandboxing, creating a path traversal risk that could be exploited via indirect prompt injection to write malicious files. This must be addressed by restricting file writes to a safe, pre-defined directory. For further improvement, consider replacing magic numbers with named constants and ensuring consistent logging practices. Also, the breaking change in the MCP tool name should be communicated to consumers.
| breadth: z.number().min(1).max(5).optional().describe("How broad to make each research level (1-5)"), | ||
| existingLearnings: z.array(z.string()).optional().describe("Optional learnings to build upon"), | ||
| goal: z.string().optional().describe("Optional goal/brief to steer synthesis"), | ||
| outputDir: z.string().optional().describe("Absolute path to save output report and learnings. Falls back to OUTPUT_DIR env var, then ./output/"), |
There was a problem hiding this comment.
The outputDir parameter allows the caller (which could be an LLM influenced by untrusted web content) to specify an absolute path for writing research results. This introduces an arbitrary file write vulnerability. An attacker could potentially use indirect prompt injection to trick the LLM into writing malicious files to sensitive locations on the user's system, such as configuration files or startup directories, depending on the permissions of the process running the MCP server.
Comment on lines
+23
to
+24
| export function resolveOutputDir(explicit?: string, fallback?: string): string { | ||
| if (explicit) return resolve(explicit); |
There was a problem hiding this comment.
The resolveOutputDir function resolves the explicit path parameter using path.resolve() without any validation or sandboxing. This allows the application to write files to any directory the process has access to. To mitigate this, you should ensure that the resolved path is contained within a designated safe base directory.
| // Insert helper functions before writeFinalReport | ||
|
|
||
| // Limit learnings to fit within a reasonable prompt window (~30K chars target) | ||
| function truncateLearnings(learnings: string[], maxChars = 30000): string[] { |
There was a problem hiding this comment.
The maxChars default value of 30000 is a magic number. For better maintainability and readability, consider defining this as a named constant at the top of the file (e.g., const MAX_LEARNINGS_CHARS_FOR_PROMPT = 30000;) and then using this constant as the default value.
| const titlePrompt = `${systemPrompt()}\n\nReturn JSON with a single 'title' for a research report based on the prompt and learnings:\nPrompt: ${prompt}\nLearnings:\n${learnings.join("\\n")}`; | ||
| const json = await callGeminiProConfigurable(titlePrompt, { json: true, schema, tools: [{ googleSearch: {} }] }); | ||
| // Title only needs a few learnings for context | ||
| const truncated = truncateLearnings(learnings, 5000); |
There was a problem hiding this comment.
The value 5000 used for truncating learnings for the title generation is a magic number. It should be defined as a named constant to improve readability and maintainability, for example: const MAX_LEARNINGS_CHARS_FOR_TITLE = 5000;.
| // Define the deep research tool (modern API) | ||
| server.registerTool( | ||
| "deepResearch.run", | ||
| "deepResearch_run", |
There was a problem hiding this comment.
Renaming the tool from deepResearch.run to deepResearch_run is a good move for robustness, as dots in tool names can sometimes cause issues. However, this is a breaking change for any clients using this tool. It would be beneficial to explicitly mention this breaking change in the pull request description or a changelog to ensure consumers of the tool are aware and can update their integrations accordingly.
| // If research returned 0 learnings due to errors, surface that clearly | ||
| if (result.learnings.length === 0 && researchErrors.length > 0) { | ||
| const errorSummary = researchErrors.join('\n- '); | ||
| const errorText = `Research failed — no learnings were collected.\n\nErrors encountered:\n- ${errorSummary}\n\nSuggestions:\n- Check your API key and billing at https://ai.dev/rate-limit\n- Free tier is limited to 20 requests/day/model\n- Try reducing depth/breadth, or wait and retry`; |
There was a problem hiding this comment.
The URL https://ai.dev/rate-limit is hardcoded in the error message. It's a good practice to avoid hardcoding external URLs in the source code. Consider moving this URL to a configuration file or an environment variable. This makes it easier to update in the future if the URL changes, without requiring a code modification and redeployment. You can define the URL constant outside this block for better readability.
Suggested change
| const errorText = `Research failed — no learnings were collected.\n\nErrors encountered:\n- ${errorSummary}\n\nSuggestions:\n- Check your API key and billing at https://ai.dev/rate-limit\n- Free tier is limited to 20 requests/day/model\n- Try reducing depth/breadth, or wait and retry`; | |
| const errorText = `Research failed — no learnings were collected.\n\nErrors encountered:\n- ${errorSummary}\n\nSuggestions:\n- Check your API key and billing at ${process.env.RATE_LIMIT_INFO_URL || 'https://ai.dev/rate-limit'}\n- Free tier is limited to 20 requests/day/model\n- Try reducing depth/breadth, or wait and retry`; |
Comment on lines
+45
to
+49
| console.warn("No valid JSON found in text."); | ||
| return null; | ||
| } catch (error) { | ||
| console.error("Error during JSON extraction:", error); | ||
| return null; |
There was a problem hiding this comment.
This function uses console.warn and console.error for logging. This is inconsistent with the rest of the application, which appears to use a centralized logger (e.g., pino via OutputManager). To maintain consistent logging and enable centralized control over log levels and outputs, consider passing a logger instance to this utility function or importing the project's logger if it doesn't create circular dependencies.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/prompt.ts (1)
190-200:⚠️ Potential issue | 🔴 CriticalModule will crash at load time due to prompt version mismatch.
validatePromptConsistency()is invoked at line 201 during module initialization and requires all templates to match the "Schema Version: X.X.X" pattern. However:
systemPromptcontains "Research Protocol: v2.3.1" (not "Schema Version")serpQueryPromptTemplatecontains no schema version at alllearningPromptTemplatecontains "Schema Version: 2.1.0"This produces
versions = [undefined, undefined, "2.1.0"], and the checknew Set(versions).size > 1evaluates totrue, throwing an error:Prompt version mismatch: undefined vs undefined vs 2.1.0.Either add matching "Schema Version: X.X.X" headers to all three templates, or update the validation logic to handle partial matches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompt.ts` around lines 190 - 200, validatePromptConsistency currently treats missing "Schema Version: X.X.X" matches as undefined and throws on mixed undefined/defined values; update validatePromptConsistency to extract versions from systemPrompt, serpQueryPromptTemplate, and learningPromptTemplate, then filter out undefined/null before comparing (e.g., const defined = versions.filter(Boolean)); if defined is empty do nothing, otherwise ensure all defined values are identical and throw a clearer error listing which templates are missing versions (reference the validatePromptConsistency function and the systemPrompt, serpQueryPromptTemplate, learningPromptTemplate identifiers).src/deep-research.ts (2)
637-647:⚠️ Potential issue | 🟠 MajorPropagate nested branch errors here.
A recursive
deepResearch()call can returnerrors, but this success path drops them. Any rate-limit/failure from lower depths disappears beforecollectedErrorsruns.♻️ Minimal fix
return { analysis: deeper.analysis, content: deeper.content, sources: deeper.sources, methodology: deeper.methodology, limitations: deeper.limitations, citations: deeper.citations.map(c => c.reference), learnings: [...newLearnings, ...deeper.learnings], visitedUrls: [...newUrls, ...deeper.visitedUrls], firecrawlResults: deeper.firecrawlResults, + errors: deeper.errors, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deep-research.ts` around lines 637 - 647, The recursive deepResearch() success return currently omits any errors from the nested call (the `deeper` object), which prevents lower-depth rate-limit/failure errors from reaching `collectedErrors`; update the returned object in deepResearch to include an `errors` field that merges the current invocation's errors with `deeper.errors` (e.g. combine whatever local/new errors array you use with `deeper.errors`) so that `collectedErrors` sees propagated errors from nested branches; look for the return that builds from `deeper` (uses symbols like deeper, newLearnings, newUrls, firecrawlResults) and add/merge the `errors` property there.
223-239:⚠️ Potential issue | 🟠 MajorPreserve Gemini's per-query
researchGoal.If the model returns
{ query, researchGoal }items, Line 236 overwrites every one with the parentresearchGoal. That collapses the branch-specific intent thatgenerateGeminiPrompt()uses later.♻️ Keep the item-level goal when it exists
.map((rawQuery: unknown) => { - const queryValue = ((): unknown => { + const { queryValue, researchGoalValue } = (() => { if (typeof rawQuery === 'object' && rawQuery !== null && 'query' in rawQuery) { - const q = (rawQuery as { query?: unknown }).query; - return typeof q === 'string' ? q : q != null ? String(q) : ''; + const item = rawQuery as { query?: unknown; researchGoal?: unknown }; + return { + queryValue: typeof item.query === 'string' ? item.query : item.query != null ? String(item.query) : '', + researchGoalValue: typeof item.researchGoal === 'string' ? item.researchGoal : researchGoal, + }; } - return rawQuery; + return { queryValue: rawQuery, researchGoalValue: researchGoal }; })(); const parsed = SerpQuerySchema.safeParse({ query: typeof queryValue === 'string' ? queryValue : String(queryValue ?? ''), - researchGoal, + researchGoal: researchGoalValue, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deep-research.ts` around lines 223 - 239, The mapping currently always injects the outer `researchGoal` into each SerpQuery, overwriting any item-level goal; update the map to detect and preserve a per-item goal when present: inside the map for `queriesArray` that builds `serpQueries`, derive an `itemResearchGoal` from `rawQuery` (e.g., if rawQuery is an object and has `researchGoal`, use that value converted to string, otherwise fallback to the outer `researchGoal`) and pass that `itemResearchGoal` into `SerpQuerySchema.safeParse` instead of the parent `researchGoal`; keep using `queryValue` as computed and validate as before so `generateGeminiPrompt()` receives the item-specific intent when available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 6-8: Remove the incorrect `.dist/` entry from .gitignore (only
`dist/` is correct per tsconfig.json and package.json) and, if the intent is to
stop tracking docs-cursorrules/, first run `git rm --cached -r
docs-cursorrules/` to untrack the already-indexed file
docs-cursorrules/code_snippets_deep_research.cursorrules before committing the
.gitignore change so the new ignore will take effect.
In `@src/deep-research.ts`:
- Around line 203-210: The catch block in generateSerpQueries is swallowing
Gemini transport/parse failures by logging and setting jsonString = '{}' which
makes deepResearch think there were no queries; instead, after logging (and
preserving the existing rate-limit throw when isRateLimitError(err)), rethrow
the error so callers like deepResearch can surface the failure (remove or avoid
the jsonString = '{}' fallback); apply the same change to the similar handling
around lines 242-243 so all non-rate-limit Gemini errors are logged and rethrown
rather than normalized to empty results.
- Around line 355-363: The truncateLearnings function currently stops scanning
when it encounters the first learning that would push totalChars over maxChars;
update truncateLearnings to skip individual oversized entries instead of
breaking so later smaller learnings can still be included: iterate over
learnings, if a single learning.length > maxChars skip it (or continue) and if
totalChars + learning.length > maxChars also continue (don’t break), otherwise
push the learning and increment totalChars; keep the function signature and
return type the same so callers of truncateLearnings behave identically.
- Around line 888-900: The fallback currently casts any extracted JSON from
extractJsonFromText into GeminiResponse (used in responseData), which can lead
to non-string payloads causing later sanitization to throw; instead validate and
normalize the extracted value against the expected GeminiResponse schema before
assigning: run the extracted value through the same normalization/validation
used by safeParseJSON (or a small schema check) to ensure response.items is an
array and each item has the expected fields with correct types (coerce or
stringify fields like learnings/summary to strings, discard or skip malformed
entries), and only then set responseData = { items: validatedItems } (keep using
safeParseJSON, extractJsonFromText, GeminiResponse, and responseData names to
locate where to add validation).
In `@src/mcp-server.ts`:
- Around line 74-79: The schema defines optional inputs goal and flags but the
async handler for MCPResearchResult doesn't destructure or use them; update the
handler signature to include goal and flags (e.g., add goal and flags to the
parameter destructuring in the async function) and either use them (pass goal to
synthesis/steering logic and use flags.grounding or flags.urlContext where
appropriate) or remove goal and flags from the zod input schema if they are
truly unused; ensure references include the schema key names goal and flags and
the async handler that currently destructures { query, depth, breadth,
existingLearnings = [], outputDir }.
In `@src/utils/errors.ts`:
- Around line 9-12: The isRateLimitError function uses a case-sensitive includes
check which can miss differently-cased API messages; update isRateLimitError to
perform case-insensitive matching against RATE_LIMIT_PATTERNS by normalizing
both the message and pattern (e.g., use const msg = (err instanceof Error ?
err.message : String(err ?? '')).toLowerCase() and compare against
RATE_LIMIT_PATTERNS.map(p => p.toLowerCase()).some(...)), or if
RATE_LIMIT_PATTERNS contains RegExp entries, use p.test(msg) and ensure those
regexes include the /i flag; refer to isRateLimitError and RATE_LIMIT_PATTERNS
when making the change.
In `@src/utils/output.ts`:
- Around line 10-14: Update the slug generation to strip both leading and
trailing dashes and provide a fallback when the result is empty: change the
chain that builds the slug (the slug variable derived from query) to use a regex
that trims dashes at both ends (for example replace(/^[-]+|[-]+$/g, '')) and
after toLowerCase check if slug === '' and, if so, assign a safe default like
'untitled' (or a sanitized/truncated version of query) so edge-case queries with
only special characters or leading punctuation never produce an empty or
dash-prefixed slug.
- Around line 48-53: The learnings JSON object currently contains both query and
goal set to the same value (opts.query); remove the redundant field by deleting
"goal: opts.query" from the object that builds the learnings payload (the block
with query, depth, breadth, goal, learnings) so only query: opts.query remains,
and update any downstream consumers if they relied on a separate goal property;
reference the fields query, goal, opts.query, and the learnings JSON object when
making the change.
---
Outside diff comments:
In `@src/deep-research.ts`:
- Around line 637-647: The recursive deepResearch() success return currently
omits any errors from the nested call (the `deeper` object), which prevents
lower-depth rate-limit/failure errors from reaching `collectedErrors`; update
the returned object in deepResearch to include an `errors` field that merges the
current invocation's errors with `deeper.errors` (e.g. combine whatever
local/new errors array you use with `deeper.errors`) so that `collectedErrors`
sees propagated errors from nested branches; look for the return that builds
from `deeper` (uses symbols like deeper, newLearnings, newUrls,
firecrawlResults) and add/merge the `errors` property there.
- Around line 223-239: The mapping currently always injects the outer
`researchGoal` into each SerpQuery, overwriting any item-level goal; update the
map to detect and preserve a per-item goal when present: inside the map for
`queriesArray` that builds `serpQueries`, derive an `itemResearchGoal` from
`rawQuery` (e.g., if rawQuery is an object and has `researchGoal`, use that
value converted to string, otherwise fallback to the outer `researchGoal`) and
pass that `itemResearchGoal` into `SerpQuerySchema.safeParse` instead of the
parent `researchGoal`; keep using `queryValue` as computed and validate as
before so `generateGeminiPrompt()` receives the item-specific intent when
available.
In `@src/prompt.ts`:
- Around line 190-200: validatePromptConsistency currently treats missing
"Schema Version: X.X.X" matches as undefined and throws on mixed
undefined/defined values; update validatePromptConsistency to extract versions
from systemPrompt, serpQueryPromptTemplate, and learningPromptTemplate, then
filter out undefined/null before comparing (e.g., const defined =
versions.filter(Boolean)); if defined is empty do nothing, otherwise ensure all
defined values are identical and throw a clearer error listing which templates
are missing versions (reference the validatePromptConsistency function and the
systemPrompt, serpQueryPromptTemplate, learningPromptTemplate identifiers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3ff6433-97c1-4977-ba85-870dfe6566ed
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.gitignoresrc/ai/providers.tssrc/deep-research.tssrc/mcp-server.tssrc/output-manager.tssrc/prompt.tssrc/run.tssrc/utils/errors.tssrc/utils/json.tssrc/utils/output.ts
Comment on lines
+6
to
+8
| # Build outputs | ||
| dist/ | ||
| dist/* | ||
| dist/**/* | ||
| dist/**/**/* | ||
| docs-cursorrrules | ||
| .dist/ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that no .dist directory is created during build and that dist/ is the actual output
# Check if .dist directory exists or is referenced anywhere
fd -HI '^\.dist$' --type d
# Check tsconfig.json for outDir configuration
rg -n '"outDir"' tsconfig.json
# Check if any build scripts or tools reference .dist (with leading dot)
rg -n '\.dist' package.json tsconfig.json --glob '!.gitignore'Repository: ssdeanx/deep-research-mcp-server
Length of output: 102
🏁 Script executed:
# Search entire codebase for .dist references (not already checked)
rg '\.dist' --type-not json --type-not lock --type-not yaml
# Check if docs-cursorrules/ exists and its git status
fd -HI '^docs-cursorrules$' --type d
git status docs-cursorrules/ 2>/dev/null || echo "Not found or not tracked"
# Also check what's currently in git index for docs-cursorrules
git ls-files | grep -E '^docs-cursorrules' | head -20Repository: ssdeanx/deep-research-mcp-server
Length of output: 216
Remove .dist/ and verify intent for docs-cursorrules/ entry.
The addition of .dist/ (line 8) is incorrect. According to tsconfig.json, the TypeScript compiler outputs to ./dist, and package.json references dist/mcp-server.js as the main entry point. A comprehensive search of the codebase confirms that .dist (with a leading dot) is never created or referenced anywhere. The patterns dist/ and .dist/ match different directories—only dist/ correctly ignores the actual build output. Remove line 8.
Additionally, docs-cursorrules/ (line 47) is already tracked in git (docs-cursorrules/code_snippets_deep_research.cursorrules is in the index). Adding it to .gitignore will not remove it from tracking. If the intent is to stop tracking this directory, use git rm --cached -r docs-cursorrules/ first before committing this .gitignore change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 6 - 8, Remove the incorrect `.dist/` entry from
.gitignore (only `dist/` is correct per tsconfig.json and package.json) and, if
the intent is to stop tracking docs-cursorrules/, first run `git rm --cached -r
docs-cursorrules/` to untrack the already-indexed file
docs-cursorrules/code_snippets_deep_research.cursorrules before committing the
.gitignore change so the new ignore will take effect.
Comment on lines
203
to
210
| } catch (err) { | ||
| output.log("Gemini error:", { error: err instanceof Error ? err.message : 'Unknown error' }); | ||
| output.log('Error in generateSerpQueries:', { error: err instanceof Error ? err.message : 'Unknown error' }); | ||
| if (isRateLimitError(err)) { | ||
| output.log('Rate limit exceeded during SERP query generation', { detail: rateLimitSummary(err) }); | ||
| throw err; | ||
| } | ||
| const errMsg = err instanceof Error ? err.message : String(err); | ||
| output.log('Gemini error:', { error: errMsg }); | ||
| jsonString = '{}'; |
There was a problem hiding this comment.
Don't turn SERP-generation failures into a successful empty run.
These branches normalize Gemini transport/parse failures to []. deepResearch() only emits errors when generateSerpQueries() throws, so auth/timeouts/bad JSON currently look like a clean "no queries" result.
♻️ Surface the failure to the caller
} catch (err) {
if (isRateLimitError(err)) {
output.log('Rate limit exceeded during SERP query generation', { detail: rateLimitSummary(err) });
throw err;
}
const errMsg = err instanceof Error ? err.message : String(err);
output.log('Gemini error:', { error: errMsg });
- jsonString = '{}';
+ throw new Error(`SERP query generation failed: ${errMsg}`);
}
@@
} else {
- output.log('Failed to generate or parse SERP queries from Gemini response, using fallback to empty array.');
- serpQueries = [];
+ throw new Error('Failed to parse SERP queries from Gemini response.');
}Also applies to: 242-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/deep-research.ts` around lines 203 - 210, The catch block in
generateSerpQueries is swallowing Gemini transport/parse failures by logging and
setting jsonString = '{}' which makes deepResearch think there were no queries;
instead, after logging (and preserving the existing rate-limit throw when
isRateLimitError(err)), rethrow the error so callers like deepResearch can
surface the failure (remove or avoid the jsonString = '{}' fallback); apply the
same change to the similar handling around lines 242-243 so all non-rate-limit
Gemini errors are logged and rethrown rather than normalized to empty results.
Comment on lines
+355
to
+363
| function truncateLearnings(learnings: string[], maxChars = 30000): string[] { | ||
| const result: string[] = []; | ||
| let totalChars = 0; | ||
| for (const learning of learnings) { | ||
| if (totalChars + learning.length > maxChars) break; | ||
| result.push(learning); | ||
| totalChars += learning.length; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Don't abort on the first oversized learning.
Line 359 breaks the entire scan. If the first learning alone exceeds maxChars, outline/report/summary/title generation runs with an empty context even when later learnings would fit.
♻️ Skip oversized entries instead of stopping the scan
for (const learning of learnings) {
- if (totalChars + learning.length > maxChars) break;
+ if (totalChars + learning.length > maxChars) continue;
result.push(learning);
totalChars += learning.length;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/deep-research.ts` around lines 355 - 363, The truncateLearnings function
currently stops scanning when it encounters the first learning that would push
totalChars over maxChars; update truncateLearnings to skip individual oversized
entries instead of breaking so later smaller learnings can still be included:
iterate over learnings, if a single learning.length > maxChars skip it (or
continue) and if totalChars + learning.length > maxChars also continue (don’t
break), otherwise push the learning and increment totalChars; keep the function
signature and return type the same so callers of truncateLearnings behave
identically.
Comment on lines
+888
to
+900
| let responseData = safeParseJSON<GeminiResponse>(geminiResponseText, { items: [] }); | ||
| if (!responseData.items || responseData.items.length === 0) { | ||
| const extracted = extractJsonFromText(geminiResponseText); | ||
| if (extracted) { | ||
| // Handle both { items: [...] } and raw array [...] formats | ||
| if (Array.isArray(extracted)) { | ||
| responseData = { items: extracted }; | ||
| } else if (extracted.items && Array.isArray(extracted.items)) { | ||
| responseData = extracted as GeminiResponse; | ||
| } else { | ||
| // Single object response — wrap it | ||
| responseData = { items: [extracted] }; | ||
| } |
There was a problem hiding this comment.
Validate extracted JSON before casting it to GeminiResponse.
This fallback trusts arbitrary extracted JSON as GeminiResponse. If Gemini emits items: [{ learnings: [1] }] or any other non-string payload, the later string sanitization throws and the whole query is discarded.
♻️ Normalize with a schema before consuming the payload
async function processGeminiResponse(geminiResponseText: string): Promise<ProcessedGeminiResponse> {
+ const GeminiResponseSchema = z.object({
+ items: z.array(z.object({
+ learning: z.string().optional(),
+ learnings: z.array(z.string()).optional(),
+ url: z.string().optional(),
+ })),
+ });
// Try direct parse first, then fall back to JSON extraction from mixed text
let responseData = safeParseJSON<GeminiResponse>(geminiResponseText, { items: [] });
if (!responseData.items || responseData.items.length === 0) {
const extracted = extractJsonFromText(geminiResponseText);
if (extracted) {
- // Handle both { items: [...] } and raw array [...] formats
- if (Array.isArray(extracted)) {
- responseData = { items: extracted };
- } else if (extracted.items && Array.isArray(extracted.items)) {
- responseData = extracted as GeminiResponse;
- } else {
- // Single object response — wrap it
- responseData = { items: [extracted] };
- }
+ const normalized =
+ Array.isArray(extracted)
+ ? { items: extracted }
+ : (typeof extracted === 'object' &&
+ extracted !== null &&
+ 'items' in extracted &&
+ Array.isArray((extracted as { items?: unknown }).items))
+ ? extracted
+ : { items: [extracted] };
+ const parsed = GeminiResponseSchema.safeParse(normalized);
+ responseData = parsed.success ? parsed.data : { items: [] };
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/deep-research.ts` around lines 888 - 900, The fallback currently casts
any extracted JSON from extractJsonFromText into GeminiResponse (used in
responseData), which can lead to non-string payloads causing later sanitization
to throw; instead validate and normalize the extracted value against the
expected GeminiResponse schema before assigning: run the extracted value through
the same normalization/validation used by safeParseJSON (or a small schema
check) to ensure response.items is an array and each item has the expected
fields with correct types (coerce or stringify fields like learnings/summary to
strings, discard or skip malformed entries), and only then set responseData = {
items: validatedItems } (keep using safeParseJSON, extractJsonFromText,
GeminiResponse, and responseData names to locate where to add validation).
Comment on lines
+74
to
+79
| goal: z.string().optional().describe("Optional goal/brief to steer synthesis"), | ||
| outputDir: z.string().optional().describe("Absolute path to save output report and learnings. Falls back to OUTPUT_DIR env var, then ./output/"), | ||
| flags: z.object({ grounding: z.boolean().optional(), urlContext: z.boolean().optional() }).optional(), | ||
| } | ||
| }, | ||
| async ({ query, depth, breadth, existingLearnings = [] }): Promise<MCPResearchResult> => { | ||
| async ({ query, depth, breadth, existingLearnings = [], outputDir }): Promise<MCPResearchResult> => { |
There was a problem hiding this comment.
Unused input parameters: goal and flags.
The input schema defines goal (line 74) and flags (line 76), but the handler (line 79) doesn't destructure or use them. Either remove from the schema or implement their functionality.
🔧 Option A: Remove unused schema fields
existingLearnings: z.array(z.string()).optional().describe("Optional learnings to build upon"),
- goal: z.string().optional().describe("Optional goal/brief to steer synthesis"),
outputDir: z.string().optional().describe("Absolute path to save output report and learnings. Falls back to OUTPUT_DIR env var, then ./output/"),
- flags: z.object({ grounding: z.boolean().optional(), urlContext: z.boolean().optional() }).optional(),
}📝 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
| goal: z.string().optional().describe("Optional goal/brief to steer synthesis"), | |
| outputDir: z.string().optional().describe("Absolute path to save output report and learnings. Falls back to OUTPUT_DIR env var, then ./output/"), | |
| flags: z.object({ grounding: z.boolean().optional(), urlContext: z.boolean().optional() }).optional(), | |
| } | |
| }, | |
| async ({ query, depth, breadth, existingLearnings = [] }): Promise<MCPResearchResult> => { | |
| async ({ query, depth, breadth, existingLearnings = [], outputDir }): Promise<MCPResearchResult> => { | |
| outputDir: z.string().optional().describe("Absolute path to save output report and learnings. Falls back to OUTPUT_DIR env var, then ./output/"), | |
| } | |
| }, | |
| async ({ query, depth, breadth, existingLearnings = [], outputDir }): Promise<MCPResearchResult> => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp-server.ts` around lines 74 - 79, The schema defines optional inputs
goal and flags but the async handler for MCPResearchResult doesn't destructure
or use them; update the handler signature to include goal and flags (e.g., add
goal and flags to the parameter destructuring in the async function) and either
use them (pass goal to synthesis/steering logic and use flags.grounding or
flags.urlContext where appropriate) or remove goal and flags from the zod input
schema if they are truly unused; ensure references include the schema key names
goal and flags and the async handler that currently destructures { query, depth,
breadth, existingLearnings = [], outputDir }.
Comment on lines
+9
to
+12
| export function isRateLimitError(err: unknown): boolean { | ||
| const msg = err instanceof Error ? err.message : String(err ?? ''); | ||
| return RATE_LIMIT_PATTERNS.some(p => msg.includes(p)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider case-insensitive matching for broader error coverage.
The includes() check is case-sensitive, but error messages from APIs can vary in casing (e.g., "Quota exceeded" vs "quota exceeded"). The regex in rateLimitSummary already uses the /i flag.
♻️ Optional: Case-insensitive pattern matching
export function isRateLimitError(err: unknown): boolean {
const msg = err instanceof Error ? err.message : String(err ?? '');
- return RATE_LIMIT_PATTERNS.some(p => msg.includes(p));
+ const msgLower = msg.toLowerCase();
+ return RATE_LIMIT_PATTERNS.some(p => msgLower.includes(p.toLowerCase()));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/errors.ts` around lines 9 - 12, The isRateLimitError function uses
a case-sensitive includes check which can miss differently-cased API messages;
update isRateLimitError to perform case-insensitive matching against
RATE_LIMIT_PATTERNS by normalizing both the message and pattern (e.g., use const
msg = (err instanceof Error ? err.message : String(err ?? '')).toLowerCase() and
compare against RATE_LIMIT_PATTERNS.map(p => p.toLowerCase()).some(...)), or if
RATE_LIMIT_PATTERNS contains RegExp entries, use p.test(msg) and ensure those
regexes include the /i flag; refer to isRateLimitError and RATE_LIMIT_PATTERNS
when making the change.
Comment on lines
+10
to
+14
| const slug = query | ||
| .slice(0, 60) | ||
| .replace(/[^a-zA-Z0-9]+/g, '-') | ||
| .replace(/-+$/, '') | ||
| .toLowerCase(); |
There was a problem hiding this comment.
Slug may start with dashes or be empty for edge-case queries.
The regex only removes trailing dashes (line 13: /-+$/), so queries starting with special characters produce slugs with leading dashes. Queries containing only special characters result in an empty slug.
🛡️ Proposed fix to handle edge cases
const slug = query
.slice(0, 60)
.replace(/[^a-zA-Z0-9]+/g, '-')
+ .replace(/^-+/, '')
.replace(/-+$/, '')
- .toLowerCase();
+ .toLowerCase() || 'research';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/output.ts` around lines 10 - 14, Update the slug generation to
strip both leading and trailing dashes and provide a fallback when the result is
empty: change the chain that builds the slug (the slug variable derived from
query) to use a regex that trims dashes at both ends (for example
replace(/^[-]+|[-]+$/g, '')) and after toLowerCase check if slug === '' and, if
so, assign a safe default like 'untitled' (or a sanitized/truncated version of
query) so edge-case queries with only special characters or leading punctuation
never produce an empty or dash-prefixed slug.
- Add validateApiKey() in providers.ts that calls models.get() to verify key - Probe quota tier with minimal generateContent call, detect free-tier from error - Log tier warning on startup (free/paid/unknown) with actionable guidance - Server still starts even if validation fails (graceful degradation)
Logger was writing to stdout, which is reserved for MCP JSON-RPC. All log output (including API key validation) was being swallowed.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/mcp-server.ts (1)
70-80:⚠️ Potential issue | 🟡 Minor
goalandflagsare still dead API surface.Lines 75-77 advertise steering inputs, but Line 80 does not destructure them and nothing downstream uses them. Either wire them into the research/report flow or remove them from the schema so the tool contract matches actual behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp-server.ts` around lines 70 - 80, The inputSchema declares goal and flags but the async handler signature async ({ query, depth, breadth, existingLearnings = [], outputDir }): Promise<MCPResearchResult> does not destructure or use them, leaving dead API surface; either remove goal and flags from inputSchema, or update the handler to destructure goal and flags (e.g., include goal and flags in the parameter object) and propagate those values into the research/report flow (pass them into the downstream functions that build the research pipeline or report generation so they affect behavior), ensuring no unused parameters remain and the declared contract matches actual usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ai/providers.ts`:
- Around line 554-580: The probe currently marks any successful
ai.models.generateContent call as 'unknown', making 'paid' unreachable; change
the success branch in the generate probe to set tier = 'paid' and tierDetail to
a message like "Key is functional and can generate content — likely a
paid/quota-enabled key; verify quotas in AI Studio/GCP Console.", while keeping
the existing catch logic (using isRateLimitError and rateLimitSummary) for
free/limited/error cases so free-tier rate-limited keys are still detected and
other errors remain 'unknown'; update the variables tier and tierDetail in the
success path of the ai.models.generateContent probe accordingly.
In `@src/mcp-server.ts`:
- Around line 80-82: The cache key generation for the research worker misses
outputDir so cached MCPResearchResult metadata.outputPath from a prior run can
be returned to a caller requesting a different outputDir; update the cacheKey
creation (the hashKey call used to produce cacheKey) to include outputDir (or,
alternatively, bypass caching when outputDir is set) and ensure the same change
is applied to the other equivalent worker code path referenced around the second
hashKey usage (lines handling existingLearnings -> MCPResearchResult
metadata.outputPath) so cached results never leak file paths to a different
output directory.
- Around line 9-19: The module imports (including validateApiKey from
./ai/providers.js) are being evaluated before environment variables are loaded,
causing providers.js to read process.env.GEMINI_API_KEY at module scope and
fail; move the dotenv config() call (the call currently using resolve(__dirname,
'../.env.local') and config) to run before any imports that depend on env vars
(i.e., before the import of validateApiKey / ./ai/providers.js) so that
providers.js sees the loaded .env.local values at module initialization.
---
Duplicate comments:
In `@src/mcp-server.ts`:
- Around line 70-80: The inputSchema declares goal and flags but the async
handler signature async ({ query, depth, breadth, existingLearnings = [],
outputDir }): Promise<MCPResearchResult> does not destructure or use them,
leaving dead API surface; either remove goal and flags from inputSchema, or
update the handler to destructure goal and flags (e.g., include goal and flags
in the parameter object) and propagate those values into the research/report
flow (pass them into the downstream functions that build the research pipeline
or report generation so they affect behavior), ensuring no unused parameters
remain and the declared contract matches actual usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa7076aa-8c54-4d61-88c3-1c956d4d7996
📒 Files selected for processing (2)
src/ai/providers.tssrc/mcp-server.ts
Comment on lines
+554
to
+580
| // Step 2: Probe quota tier with a minimal generate call | ||
| let tier: 'free' | 'paid' | 'unknown' = 'unknown'; | ||
| let tierDetail: string | undefined; | ||
| try { | ||
| await ai.models.generateContent({ | ||
| model, | ||
| contents: 'ping', | ||
| config: { maxOutputTokens: 1 }, | ||
| }); | ||
| // If we get here, the key works for generation. We can't definitively | ||
| // determine tier from a success, but at least it's functional. | ||
| tier = 'unknown'; | ||
| tierDetail = 'Key is functional. Tier cannot be determined from a successful call — check AI Studio or GCP Console for quota details.'; | ||
| } catch (err: unknown) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| if (msg.includes('free_tier') || msg.includes('FreeTier')) { | ||
| tier = 'free'; | ||
| tierDetail = 'API key is on FREE TIER. Rate limits: ~20 req/day/model. Link billing in GCP Console to upgrade.'; | ||
| } else if (isRateLimitError(err)) { | ||
| // Rate limited but not explicitly free-tier — could be either tier | ||
| tier = 'unknown'; | ||
| tierDetail = `Rate limited on probe: ${rateLimitSummary(err)}. This may indicate free-tier or heavy usage.`; | ||
| } else { | ||
| // Some other error on generate — key itself validated via models.get above | ||
| tier = 'unknown'; | ||
| tierDetail = `Generation probe returned: ${msg.slice(0, 200)}`; | ||
| } |
There was a problem hiding this comment.
The quota probe never identifies a working free-tier or paid key.
Lines 563-566 classify every successful generateContent probe as unknown, so paid is unreachable and a normal free-tier key also stays unknown until it is already rate-limited. That means the startup warning misses the exact case this API is trying to surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai/providers.ts` around lines 554 - 580, The probe currently marks any
successful ai.models.generateContent call as 'unknown', making 'paid'
unreachable; change the success branch in the generate probe to set tier =
'paid' and tierDetail to a message like "Key is functional and can generate
content — likely a paid/quota-enabled key; verify quotas in AI Studio/GCP
Console.", while keeping the existing catch logic (using isRateLimitError and
rateLimitSummary) for free/limited/error cases so free-tier rate-limited keys
are still detected and other errors remain 'unknown'; update the variables tier
and tierDetail in the success path of the ai.models.generateContent probe
accordingly.
The SDK swallows response headers, so tier detection via generateContent always returned 'unknown'. Now makes a raw fetch to the REST API and inspects x-ratelimit-limit-requests headers to classify free (<= 50) vs paid tier. Falls back to error body parsing for exhausted quotas.
…efully Google's Gemini API does not return x-ratelimit-* headers on responses. Updated probe logic to: - Future-proof header detection (if Google adds them later) - Detect free tier from error body keywords (free_tier/FreeTier) - Detect expired/invalid keys from REST probe even when SDK works - Parse usageMetadata from successful responses - Provide clear guidance to check AI Studio for tier info
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/mcp-server.ts (2)
80-82:⚠️ Potential issue | 🟠 MajorInclude
outputDirin the cache key to avoid path mismatches.The cache key excludes
outputDir, so a second request with identical query parameters but a differentoutputDirwill return the cachedmetadata.outputPathpointing to the first caller's directory — and no files will be written to the requested location.🔧 Proposed fix
async ({ query, depth, breadth, existingLearnings = [], outputDir }): Promise<MCPResearchResult> => { + // Resolve output dir early so it can be included in cache key + const resolvedOutputDir = resolveOutputDir(outputDir, resolve(__dirname, '../output')); + // 1. Create cache key - const cacheKey = hashKey({ query, depth, breadth, existingLearnings }); + const cacheKey = hashKey({ query, depth, breadth, existingLearnings, outputDir: resolvedOutputDir });Then remove the duplicate
resolveOutputDircall at line 138.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp-server.ts` around lines 80 - 82, The cache key created by hashKey({ query, depth, breadth, existingLearnings }) omits outputDir, causing cached metadata.outputPath to point to the wrong directory; update the cacheKey to include outputDir (i.e., hashKey({ query, depth, breadth, existingLearnings, outputDir })) and then remove the duplicated resolveOutputDir call (the second resolveOutputDir invocation) so output path resolution only happens once; ensure references to metadata.outputPath are consistent with the resolved outputDir.
75-80:⚠️ Potential issue | 🟡 MinorUnused input parameters:
goalandflags.The schema defines
goal(line 75) andflags(line 77), but the handler on line 80 does not destructure or use them. Either remove them from the schema or implement their functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp-server.ts` around lines 75 - 80, The handler signature for the async function returning Promise<MCPResearchResult> declares and uses { query, depth, breadth, existingLearnings = [], outputDir } but the zod schema also defines goal and flags which are unused; either add goal and flags to the handler destructuring (async ({ query, depth, breadth, existingLearnings = [], outputDir, goal, flags }): Promise<MCPResearchResult> => ...) and propagate them into whatever synthesis/research functions or result construction that needs steering or runtime flags, or remove goal and flags from the schema entirely if they are not required; make sure any downstream calls that rely on goal/flags are updated to accept those parameters when you add them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp-server.ts`:
- Around line 9-19: The import of validateApiKey from providers.js runs before
dotenv is loaded because ESM imports are hoisted; fix by loading env first and
then dynamically importing the providers module (e.g., call
config({...})/resolveOutputDir/whatever env setup, then await
import('./ai/providers.js') to obtain validateApiKey) so GEMINI_API_KEY is
present when providers initializes; alternatively, remove module-scope throws in
providers.ts and switch to lazy initialization (e.g., export a function that
reads/validates the key on first use) and update usages to call that function
instead.
---
Duplicate comments:
In `@src/mcp-server.ts`:
- Around line 80-82: The cache key created by hashKey({ query, depth, breadth,
existingLearnings }) omits outputDir, causing cached metadata.outputPath to
point to the wrong directory; update the cacheKey to include outputDir (i.e.,
hashKey({ query, depth, breadth, existingLearnings, outputDir })) and then
remove the duplicated resolveOutputDir call (the second resolveOutputDir
invocation) so output path resolution only happens once; ensure references to
metadata.outputPath are consistent with the resolved outputDir.
- Around line 75-80: The handler signature for the async function returning
Promise<MCPResearchResult> declares and uses { query, depth, breadth,
existingLearnings = [], outputDir } but the zod schema also defines goal and
flags which are unused; either add goal and flags to the handler destructuring
(async ({ query, depth, breadth, existingLearnings = [], outputDir, goal, flags
}): Promise<MCPResearchResult> => ...) and propagate them into whatever
synthesis/research functions or result construction that needs steering or
runtime flags, or remove goal and flags from the schema entirely if they are not
required; make sure any downstream calls that rely on goal/flags are updated to
accept those parameters when you add them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 940d329c-6c71-462c-b538-7d44ab7371a8
📒 Files selected for processing (2)
src/ai/providers.tssrc/mcp-server.ts
Comment on lines
+9
to
+19
| import { validateApiKey } from './ai/providers.js'; | ||
| import { LRUCache } from 'lru-cache'; | ||
| import { logger } from './logger.js'; | ||
| import { resolveOutputDir, writeResearchOutput } from './utils/output.js'; | ||
|
|
||
|
|
||
| // Get the directory name of the current module | ||
| const __dirname = fileURLToPath(new URL('.', import.meta.url)); | ||
|
|
||
| // Load environment variables from .env.local | ||
| config({ path: resolve(__dirname, '../.env.local') }); | ||
| config({ path: resolve(__dirname, '../.env.local'), override: true }); |
There was a problem hiding this comment.
ESM hoisting defeats static config() placement — use dynamic import.
The past review correctly identified that providers.ts throws at module scope if GEMINI_API_KEY is missing. However, simply moving config() before the import statement won't work in ESM — all import declarations are hoisted and evaluated before any module code runs.
To ensure .env.local is loaded before providers.ts reads GEMINI_API_KEY, use a dynamic import:
🔧 Proposed fix using dynamic import
import { config } from 'dotenv';
import { resolve } from 'node:path';
import { fileURLToPath } from 'node:url';
+
+const __dirname = fileURLToPath(new URL('.', import.meta.url));
+
+// Load environment variables BEFORE importing modules that depend on them
+config({ path: resolve(__dirname, '../.env.local'), override: true });
+
import { createHash } from 'node:crypto';
import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";
import { z } from "zod";
import { research, writeFinalReport, type ResearchProgress, type ResearchOptions } from "./deep-research.js";
-import { validateApiKey } from './ai/providers.js';
+const { validateApiKey } = await import('./ai/providers.js');
import { LRUCache } from 'lru-cache';
import { logger } from './logger.js';
import { resolveOutputDir, writeResearchOutput } from './utils/output.js';
-
-
-// Get the directory name of the current module
-const __dirname = fileURLToPath(new URL('.', import.meta.url));
-
-// Load environment variables from .env.local
-config({ path: resolve(__dirname, '../.env.local'), override: true });Alternatively, refactor providers.ts to lazy-initialize the API key check instead of throwing at module scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp-server.ts` around lines 9 - 19, The import of validateApiKey from
providers.js runs before dotenv is loaded because ESM imports are hoisted; fix
by loading env first and then dynamically importing the providers module (e.g.,
call config({...})/resolveOutputDir/whatever env setup, then await
import('./ai/providers.js') to obtain validateApiKey) so GEMINI_API_KEY is
present when providers initializes; alternatively, remove module-scope throws in
providers.ts and switch to lazy initialization (e.g., export a function that
reads/validates the key on first use) and update usages to call that function
instead.
…ver .env.local When the MCP host (VS Code) passes GEMINI_API_KEY via mcp.json env, it should take precedence over .env.local which may have stale keys. Changed from override:true to override:false so .env.local is only a fallback when no env var is already set.
- Fix feedback crash on empty conductResearch analysis - Fix impossible validateAcademicOutput check in feedback path - Fix MCP cache key missing goal/outputDir fields - Fix URL dedupe comparing query strings instead of URLs - Fix double progress counting in recursive research - Skip no-op Firecrawl processSerpResult on empty data - Fix output-manager timer never resetting after flush - Fix divide-by-zero in progress bars (terminal-utils, output-manager) - Align feedbackPromptTemplate schema with Zod/JSON schema (strings not objects) - Relax node engine requirement to >=20
… learnings dump - Remove 'Key Learnings' section that duplicated report body content - Remove boilerplate 'Methodology' and 'Limitations' sections - Instruct LLM to produce inline numbered citations [1], [2] etc. - Pass source URLs to report writer for proper citation linking - Generate formatted bibliography from cited sources - Update system prompt to guide academic paper structure - Update outline generation for thematic academic sections
- Add strict writing style constraints to report body prompt - Add paragraph-only instruction to summary generation - Update system prompt with mandatory style rules - Prefer commas, semicolons, parentheses over dashes
- Add md-to-pdf dependency for automated PDF conversion - Prepend YAML frontmatter (A4, Georgia serif header/footer) to reports - Auto-copy report-style.css to output directory - Generate PDF alongside markdown after each research run - Add pdfPath to MCP response metadata - Move report-style.css to tracked assets/ directory - PDF generation is best-effort (non-blocking on failure)
Add SourcedLearning interface (text + sourceUrl) to preserve the link between extracted learnings and their source URLs throughout the research pipeline. - processGeminiResponse pairs each learning with its source URL - ProcessResult and ResearchResult carry sourcedLearnings arrays - writeReportFromOutline annotates findings with [source: N] markers and builds a deduplicated numbered source list for the LLM - All entry points (MCP server, HTTP server, CLI) pass sourcedLearnings through to writeFinalReport - Falls back to flat learnings + URL list when no sourcedLearnings exist
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| breadth: z.number().min(1).max(5).optional().describe("How broad to make each research level (1-5)"), | ||
| existingLearnings: z.array(z.string()).optional().describe("Optional learnings to build upon"), | ||
| goal: z.string().optional().describe("Optional goal/brief to steer synthesis"), | ||
| outputDir: z.string().optional().describe("Absolute path to save output report and learnings. Falls back to OUTPUT_DIR env var, then ./output/"), |
There was a problem hiding this comment.
CRITICAL: Path traversal vulnerability - outputDir allows arbitrary absolute paths without validation
The outputDir parameter accepts any string and passes it directly to resolve(), enabling path traversal attacks. An attacker could specify paths like '../../../etc/passwd' to write files outside intended directories. Add path validation to ensure the resolved path stays within allowed directories.
|
|
||
| const httpServer = createServer(async (req, res) => { | ||
| // CORS | ||
| res.setHeader('Access-Control-Allow-Origin', process.env.CORS_ORIGIN || '*'); |
There was a problem hiding this comment.
WARNING: Overly permissive CORS configuration
Setting Access-Control-Allow-Origin to '*' allows requests from any origin, which could be a security risk if the server is exposed beyond localhost. Consider restricting to specific allowed origins or using environment variable validation.
Code Review SummaryStatus: Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (14 files)
Fix these issues in Kilo Cloud Reviewed by grok-code-fast-1:optimized:free · 433,028 tokens |
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
Code quality cleanup and deduplication across the codebase.
Changes
console.error('[DBG]')debug statements acrossproviders.tsanddeep-research.tsisRateLimitError()andrateLimitSummary()to newsrc/utils/errors.ts, replacing 6+ copy-pasted inline checksresolveOutputDir(),buildOutputPaths(),writeResearchOutput()to newsrc/utils/output.ts, deduplicating code betweenmcp-server.tsandrun.tscallGeminiProConfigurable()— remove redundant try/catch wrapper (errors already propagate fromgenerateContentInternal)truncateLearnings()from module-leveloutputobjectprocessGeminiResponse().gitignore— deduplicate entries (5xdist/), fix typos, organize into sectionssrc/utils/json.tssrc/prompt.tsFiles Changed (11)
src/utils/errors.tssrc/utils/output.tssrc/ai/providers.tssrc/deep-research.tssrc/mcp-server.tssrc/run.tssrc/output-manager.tssrc/prompt.tssrc/utils/json.ts.gitignorepackage-lock.jsonTesting
tsc --noEmitpasses with exit code 0)Summary by Sourcery
Refactor deep research, provider, and MCP server flows to improve JSON handling, centralize rate-limit and output management, and surface structured errors and outputs to callers.
New Features:
Bug Fixes:
Enhancements:
Chores: