[Feature] refactor and harden search browser tooling#864
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough引入 PlatformService 构造注入；用 BrowserManager + 细粒度 browser_* 工具替代旧 PuppeteerBrowserTool，重构搜索链、工具注册、配置与搜索动作解析以适配新浏览器体系。 Changes平台服务构造注入
浏览器工具系统重构
搜索服务集成与工具适配
文档与扩展配置更新
Sequence DiagramsequenceDiagram
participant Client
participant ChatLunaBrowsingChain
participant BrowserManager
participant PuppeteerPage
participant ChatLunaChatModel
Client->>ChatLunaBrowsingChain: 提交搜索请求/问题
ChatLunaBrowsingChain->>BrowserManager: 请求 readText / summarize 或调度 web_search
BrowserManager->>PuppeteerPage: 打开/导航/执行 DOM 抽取
PuppeteerPage-->>BrowserManager: 返回文本/HTML/链接/快照
BrowserManager->>ChatLunaChatModel: 若需要摘要，构造提示并调用模型.invoke
ChatLunaChatModel-->>BrowserManager: 返回摘要文本
BrowserManager-->>ChatLunaBrowsingChain: 返回读取/摘要结果
ChatLunaBrowsingChain-->>Client: 追加上下文并继续对话流程
🎯 4 (Complex) | ⏱️ ~60 minutes 可能相关的 PR:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request replaces the legacy web_browser tool with a comprehensive BrowserManager and a suite of granular browser_* tools, updating the Sydney preset, agent skills, and browsing chain accordingly. The review feedback highlights several critical concerns: the use of Promise.all in search operations could cause total failure if a single URL fails; security vulnerabilities exist where the LLM can specify arbitrary file paths for screenshots or uploads; potential infinite loops and resource leaks were identified in browser page management and file storage; and the use of locale-dependent date strings in prompts may lead to inconsistent behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bbec08d37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/extension-agent/resources/skills/sub-agent-creator/SKILL.md`:
- Around line 39-40: 在“Web research agents” 的默认工具列表中补入缺失的 browser_open，使默认集合包含
browser_open、web_search、browser_read_text 和 browser_summarize；打开 SKILL.md 中“Web
research agents”相关段落并在列举 web_search、browser_read_text、browser_summarize 的同一条目中新增
browser_open（保持与其它工具名称相同的格式和标点），以确保示例配置能先打开页面再执行后续浏览与摘要步骤。
In `@packages/service-search/src/chain/browsing_chain.ts`:
- Around line 398-422: The current use of Promise.all inside raceAbort causes a
fail-fast behavior that discards all results if any question/tool.invoke
rejects; update the block that builds results (the Promise.all over questions
invoking tool.invoke in browsing_chain.ts) to use Promise.allSettled instead,
then extract only the fulfilled values, JSON.parse each fulfilled value into
SearchResultLike[], and merge/return those while skipping rejected entries
(optionally logging or session.send errors); apply the same change to the
similar block around the 441-461 range so failures for individual questions/URLs
don't wipe out all results.
- Line 518: Summary: Fix grammatical errors in the system prompt string that
uses "your's". Locate the prompt string in browsing_chain.ts (the system prompt
/ message text used in the browsing chain) and replace the incorrect "your's"
occurrences with the correct possessive "your" and remove the extra "the" so the
sentence reads clearly (e.g., "OK. I understand. I will respond to your question
using the same language as your input. What's your question?"). Ensure both
instances in that string are updated and run a quick lint or test to verify no
other prompt formatting was altered.
- Around line 557-566: The raceAbort function doesn't immediately reject when
the provided AbortSignal is already aborted; update raceAbort to first check
signal?.aborted and synchronously reject with new
ChatLunaError(ChatLunaErrorCode.ABORTED) if true, otherwise attach the 'abort'
listener as before and ensure the listener is removed in the finally block;
reference the raceAbort function and the AbortSignal usage
(signal?.addEventListener, signal?.removeEventListener) so the change is applied
in that function.
In `@packages/service-search/src/tools/browser/manager.ts`:
- Around line 359-366: The cloned snapshot nodes lose prototype methods (like
elementHandle) because assignSnapshotIds uses Object.assign({}, raw, ...) which
copies only own properties; update assignSnapshotIds to preserve the original
prototype when cloning nodes—e.g. create the clone with
Object.create(Object.getPrototypeOf(raw)) and then Object.assign(clone, raw, {
uid: ... }) or explicitly bind/copy the elementHandle method
(clone.elementHandle = raw.elementHandle.bind(raw)); ensure this change
preserves elementHandle so getElement, browser_click, browser_fill,
browser_upload_file, etc. can call node.elementHandle() successfully.
In `@packages/service-search/src/tools/browser/tools.ts`:
- Around line 209-230: The schema declares url is required for action 'url' but
doesn't enforce it; update the zod schema (the schema built from pageIdSchema)
to conditionally require url when action === 'url' (e.g., use
z.discriminatedUnion or .refine/when on action to enforce url presence), and add
a runtime guard in the _call method to throw a clear error if input.action ===
'url' and !input.url before calling BrowserManager.navigate so invalid input is
rejected at the tool boundary.
- Around line 396-416: The uid branch acquires an element handle via
this.manager.getElement but never releases it, causing remote handle leaks;
modify the screenshot flow around getElement/target.screenshot to mirror other
tools by using a try/finally: obtain page via this.manager.getPage, if input.uid
call this.manager.getElement and store the handle in a local (e.g.
targetHandle), use it for targetHandle.screenshot, and in the finally block
release the handle (either await targetHandle.dispose() or call the manager's
release method if one exists) only when a handle was created (i.e. when target
is not the page.page) so long-running sessions don't accumulate element handles.
In `@packages/service-search/src/tools/search.ts`:
- Around line 83-90: The current Promise.all over results calling
this.createDocuments(result, query, llm, runConfig) will reject the whole batch
if any single browser/read call throws; replace the all-or-nothing behavior by
making each call resilient — either use Promise.allSettled and keep only
fulfilled values (flattening their arrays) or wrap each invocation in a
try/catch that returns an empty array or a fallback descriptive document on
error; apply the same defensive change to the other bulk call in this file that
follows the same pattern so single-page read failures are skipped instead of
failing the entire retrieval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 319f96a1-2b8a-4cb8-9223-02ac7fb92446
⛔ Files ignored due to path filters (3)
packages/core/resources/presets/sydney.ymlis excluded by!**/*.ymlpackages/service-search/src/locales/en-US.schema.ymlis excluded by!**/*.ymlpackages/service-search/src/locales/zh-CN.schema.ymlis excluded by!**/*.yml
📒 Files selected for processing (18)
packages/adapter-zhipu/src/types.tspackages/core/src/services/chat.tspackages/core/src/services/conversation.tspackages/core/tests/helpers.tspackages/extension-agent/resources/skills/sub-agent-creator/SKILL.mdpackages/extension-agent/src/config/defaults.tspackages/extension-agent/src/sub-agent/parse.tspackages/service-search/src/chain/browsing_chain.tspackages/service-search/src/config.tspackages/service-search/src/index.tspackages/service-search/src/plugin.tspackages/service-search/src/provide.tspackages/service-search/src/providers/bing_api.tspackages/service-search/src/tools/browser/manager.tspackages/service-search/src/tools/browser/tools.tspackages/service-search/src/tools/puppeteerBrowserTool.tspackages/service-search/src/tools/search.tspackages/service-search/src/utils/parse.ts
💤 Files with no reviewable changes (1)
- packages/service-search/src/tools/puppeteerBrowserTool.ts
dingyi222666
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/service-search/src/tools/browser/manager.ts`:
- Around line 326-331: The summary flow currently calls readText(), which runs
formatOutput() and returns a truncated "Output too large..." preview to
model.invoke(createSummaryPrompt...), causing summaries to miss content; change
manager.ts to bypass readText()/formatOutput() for summary generation by using
the raw extracted page text (the original extractor result) as the input to
createSummaryPrompt/model.invoke so the model receives the full page text, and
keep truncation only when preparing the user-facing return value (i.e., apply
browserOutputLimit / formatOutput only on the output shown to users, not on the
prompt passed to model.invoke).
In `@packages/service-search/src/tools/browser/tools.ts`:
- Around line 615-622: The sequence that presses modifier keys via
this.manager.getPage(...).page.keyboard.down(...) then keyboard.press(...) must
ensure all modifiers are released even if an error occurs; wrap the press call
and any preceding down calls in a try/finally where the finally iterates the
original modifiers in reverse order and calls page.page.keyboard.up(...) for
each (use a reversed copy of keys to avoid mutating the original array).
Specifically, keep using getPage and the keyboard.down/press/up calls but move
the press into the try block and perform the up calls inside finally so
Ctrl/Shift/Alt cannot leak to later actions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 81d5a3f5-c7e0-49a0-a98d-71194551eaf8
⛔ Files ignored due to path filters (26)
packages/adapter-azure-openai/package.jsonis excluded by!**/*.jsonpackages/adapter-claude/package.jsonis excluded by!**/*.jsonpackages/adapter-deepseek/package.jsonis excluded by!**/*.jsonpackages/adapter-dify/package.jsonis excluded by!**/*.jsonpackages/adapter-doubao/package.jsonis excluded by!**/*.jsonpackages/adapter-gemini/package.jsonis excluded by!**/*.jsonpackages/adapter-hunyuan/package.jsonis excluded by!**/*.jsonpackages/adapter-ollama/package.jsonis excluded by!**/*.jsonpackages/adapter-openai-like/package.jsonis excluded by!**/*.jsonpackages/adapter-openai/package.jsonis excluded by!**/*.jsonpackages/adapter-qwen/package.jsonis excluded by!**/*.jsonpackages/adapter-rwkv/package.jsonis excluded by!**/*.jsonpackages/adapter-spark/package.jsonis excluded by!**/*.jsonpackages/adapter-wenxin/package.jsonis excluded by!**/*.jsonpackages/adapter-zhipu/package.jsonis excluded by!**/*.jsonpackages/core/package.jsonis excluded by!**/*.jsonpackages/extension-agent/package.jsonis excluded by!**/*.jsonpackages/extension-long-memory/package.jsonis excluded by!**/*.jsonpackages/extension-tools/package.jsonis excluded by!**/*.jsonpackages/extension-variable/package.jsonis excluded by!**/*.jsonpackages/renderer-image/package.jsonis excluded by!**/*.jsonpackages/service-embeddings/package.jsonis excluded by!**/*.jsonpackages/service-multimodal/package.jsonis excluded by!**/*.jsonpackages/service-search/package.jsonis excluded by!**/*.jsonpackages/service-vector-store/package.jsonis excluded by!**/*.jsonpackages/shared-adapter/package.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
packages/extension-agent/resources/skills/sub-agent-creator/SKILL.mdpackages/extension-agent/src/config/defaults.tspackages/service-search/src/chain/browsing_chain.tspackages/service-search/src/index.tspackages/service-search/src/tools/browser/manager.tspackages/service-search/src/tools/browser/tools.tspackages/service-search/src/tools/search.ts
✅ Files skipped from review due to trivial changes (1)
- packages/extension-agent/resources/skills/sub-agent-creator/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/service-search/src/index.ts
- packages/service-search/src/tools/search.ts
- packages/service-search/src/chain/browsing_chain.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/service-search/src/tools/browser/manager.ts`:
- Around line 183-198: The new page created by createPage() is left in
session.pages and set as selected even if page.page.goto(...) fails; wrap the
navigation in a try/catch so you only commit the new page on successful
navigation: save the previous selectedPageId before createPage(), call
createPage(), then try { await page.page.goto(...); session.selectedPageId =
page.id; await this.trimPages(session) } catch (err) { await page.close();
session.pages.delete(page.id); session.selectedPageId = previousSelectedId;
throw err } — reference getSession, createPage, page.page.goto,
session.selectedPageId and trimPages to locate the code to update.
- Around line 566-570: The loop currently skips any anchor with url.hash set,
which wrongly excludes external or different-path URLs with fragments; change
the logic to only skip same-document fragment links by parsing current into a
URL (e.g., const currentUrl = new URL(current)) and then continue only when
url.hash is non-empty AND url.origin === currentUrl.origin AND url.pathname ===
currentUrl.pathname AND url.search === currentUrl.search; keep the other checks
(like url.href === current) as needed. This targets the code around the anchor
iteration using document.querySelectorAll('a[href]'), the local variable url =
new URL(...), and the url.hash check.
In `@packages/service-search/src/tools/browser/tools.ts`:
- Around line 424-431: The computed filename can have an extension that
disagrees with the actual screenshot encoding (input.format →
target.screenshot({ type: format })), so change the rawName/name logic to force
the extension to match input.format: derive rawName from input.filePath if
present, strip any existing extension, append `.` + format, then sanitize into
name (keep existing regex replace). Also validate input.format is a supported
type and throw or reject if format is missing/invalid; apply the same fix to the
other occurrence handling the screenshot output (the block referenced around the
second occurrence).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 74debe63-325f-4a1b-84bc-73796b600382
📒 Files selected for processing (2)
packages/service-search/src/tools/browser/manager.tspackages/service-search/src/tools/browser/tools.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/service-search/src/tools/browser/tools.ts`:
- Around line 334-344: In _call (async _call) ensure the model is validated
before calling this.manager.summarize: check this.model.value and
cfg.configurable.model and if both are undefined throw a clear error (or choose
a sensible default) rather than passing undefined into this.manager.summarize;
update the code around the model selection so you compute a non-undefined model
variable first (e.g., const model = this.model.value ?? cfg.configurable.model)
and validate it, then pass that validated model into this.manager.summarize to
avoid runtime failures inside model.invoke().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7eae474-cf19-4edc-8e73-e9ef00bae120
📒 Files selected for processing (2)
packages/service-search/src/tools/browser/manager.tspackages/service-search/src/tools/browser/tools.ts
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.
This pr refactors the search service browser tooling into reusable browser manager and tool modules, while updating search configuration, agent integration, and browser tool safety for the new search flow.
New Features
Bug fixes
Other Changes