[Fix] align agent behavior and conversation lifecycle hooks#824
Merged
dingyi222666 merged 14 commits intov1-devfrom Apr 11, 2026
Merged
[Fix] align agent behavior and conversation lifecycle hooks#824dingyi222666 merged 14 commits intov1-devfrom
dingyi222666 merged 14 commits intov1-devfrom
Conversation
Keep emitters, listeners, and specs in sync after the hook rename so cleanup handlers continue to run across core and extensions.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 refactors event naming conventions across the core and extension packages, renaming 'chatluna/conversation-before-' and 'chatluna/conversation-after-' events to 'chatluna/before-conversation-' and 'chatluna/after-conversation-' respectively. It also removes the 'file_sender' plugin and its related configurations from 'extension-tools'. Other changes include adding a blacklist check in the 'allow_reply' middleware and updating the 'open_terminal' backend to use a new file viewing endpoint. Review feedback identifies potential runtime errors in the permission service where the 'source' parameter was made optional but is passed to functions requiring a string; adding default values is recommended to ensure stability.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough统一重命名并同步会话生命周期事件名；在 agent 运行链中注入 AgentRunContext 并新增代理事件发射；为会话请求加入活动触摸与超时中止；增强队列并发窗口与超时；移除 file_sender 并将多个工具由类重构为 tool(...)；新增远程运行时同步服务与若干类型/权限调整。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant ChatService as ChatLunaService
participant ConvRuntime as ConversationRuntime
participant AgentRunner as AgentRunner
participant LegacyExec as LegacyAgentExecutor
participant RuntimeSync as RuntimeSyncService
participant Computer as ComputerBackend
participant FS as Filesystem
Client->>ChatService: 调用 chat(..., callbacks?, requestId?)
ChatService->>ConvRuntime: forward chat(..., callbacks)
ConvRuntime->>AgentRunner: 启动 agent run（含 configurable.agentContext）
AgentRunner->>LegacyExec: 运行 agent，产生 AgentEvent
LegacyExec->>AgentRunner: emitAgentEvent -> runManager.handleCustomEvent (CHATLUNA_AGENT_EVENT)
AgentRunner->>RuntimeSync: 注册/计数运行（AgentRunContext）
RuntimeSync->>Computer: getAgentSession(context)
Computer->>FS: 远端/本地 读写 文件（同步回写）
RuntimeSync->>AgentRunner: 运行结束后触发 reload（如有变更）
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 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 |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-agent/src/service/permissions.ts (1)
313-326:⚠️ Potential issue | 🔴 Critical
source参数类型不匹配：应接受可选值。
isSessionAllowed的source参数声明为必需的string，但ToolMaskArg接口定义source?: string为可选。第 39-44 行的回调函数接收可能为undefined的source，在第 44 行显式传入createMainToolMask(session, source)。由于显式传递undefined不会触发默认值，此值流向第 283 行的isSessionAllowed调用，导致类型错误。修复类型定义
isSessionAllowed( session: Session | undefined, - source: string, + source: string | undefined, item: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/permissions.ts` around lines 313 - 326, isSessionAllowed currently declares source as a required string but callers (e.g., the callback that calls createMainToolMask(session, source) and the ToolMaskArg interface) can pass undefined; change the isSessionAllowed signature to accept source?: string (or source: string | undefined) and update any internal handling inside isSessionAllowed (and related calls like createMainToolMask) to handle undefined safely so the types align with ToolMaskArg and caller sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/extension-agent/src/service/permissions.ts`:
- Around line 313-326: isSessionAllowed currently declares source as a required
string but callers (e.g., the callback that calls createMainToolMask(session,
source) and the ToolMaskArg interface) can pass undefined; change the
isSessionAllowed signature to accept source?: string (or source: string |
undefined) and update any internal handling inside isSessionAllowed (and related
calls like createMainToolMask) to handle undefined safely so the types align
with ToolMaskArg and caller sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 40d9c646-5eda-41f3-824a-01b449581d1f
⛔ Files ignored due to path filters (3)
.github/workflows/build.ymlis excluded by!**/*.ymlpackages/extension-tools/src/locales/en-US.schema.ymlis excluded by!**/*.ymlpackages/extension-tools/src/locales/zh-CN.schema.ymlis excluded by!**/*.yml
📒 Files selected for processing (21)
packages/core/src/llm-core/chat/app.tspackages/core/src/llm-core/memory/authors_note/index.tspackages/core/src/llm-core/memory/lore_book/index.tspackages/core/src/llm-core/platform/service.tspackages/core/src/llm-core/prompt/context_manager.tspackages/core/src/middlewares/chat/allow_reply.tspackages/core/src/middlewares/chat/message_delay.tspackages/core/src/services/conversation.tspackages/core/src/services/conversation_runtime.tspackages/core/tests/conversation-service.spec.tspackages/core/tests/conversation-source.spec.tspackages/extension-agent/src/cli/service.tspackages/extension-agent/src/computer/backends/open_terminal.tspackages/extension-agent/src/config/defaults.tspackages/extension-agent/src/service/permissions.tspackages/extension-agent/src/service/skills.tspackages/extension-long-memory/src/service/memory.tspackages/extension-tools/src/config.tspackages/extension-tools/src/plugin.tspackages/extension-tools/src/plugins/file_sender.tspackages/extension-tools/src/plugins/todos.ts
💤 Files with no reviewable changes (2)
- packages/extension-agent/src/config/defaults.ts
- packages/extension-tools/src/plugins/file_sender.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-agent/src/service/permissions.ts (1)
315-367:⚠️ Potential issue | 🟠 Major修复
isSessionAllowed中的未知来源处理
source参数从'chatluna' | 'character'放宽到string后，逻辑未相应更新。当前只有source === 'chatluna'的特判，其余所有值都会无条件进入 character 权限检查。若未来有未预期的来源值传入，会被错误地以 character 规则评估，导致权限结果偏离预期。建议显式处理已知来源，拒绝未知来源：
建议修改
if (source === 'chatluna') { return item.chatlunaEnabled } + + if (source !== 'character') { + return false + } if (!item.characterEnabled) { return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/permissions.ts` around lines 315 - 367, The isSessionAllowed function currently only special-cases source === 'chatluna' and otherwise always applies character rules, which mis-handles unknown source values; update isSessionAllowed to explicitly branch on known sources (e.g., if source === 'chatluna' return item.chatlunaEnabled; else if source === 'character' run the existing character permission logic that uses session?.isDirect, characterPrivateEnabled, characterGroupEnabled, modes and id resolution) and return false for any other/unknown source values to reject unexpected origins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-agent/src/service/permissions.ts`:
- Line 270: The single-line function signature for createMainToolMask(session?:
Session, source: string = 'chatluna'): ToolMask is exceeding Prettier's max line
width and causing CI failures; rewrite the signature into a multi-line format
(one parameter per line, default value on the same parameter line if needed),
align the closing parenthesis and the return type on a new line, and apply the
same multi-line signature style to the other long signature around line 370 so
both match Prettier formatting rules.
---
Outside diff comments:
In `@packages/extension-agent/src/service/permissions.ts`:
- Around line 315-367: The isSessionAllowed function currently only
special-cases source === 'chatluna' and otherwise always applies character
rules, which mis-handles unknown source values; update isSessionAllowed to
explicitly branch on known sources (e.g., if source === 'chatluna' return
item.chatlunaEnabled; else if source === 'character' run the existing character
permission logic that uses session?.isDirect, characterPrivateEnabled,
characterGroupEnabled, modes and id resolution) and return false for any
other/unknown source values to reject unexpected origins.
🪄 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: 520e57fc-39b9-4c58-b9c2-70ac06e20563
📒 Files selected for processing (1)
packages/extension-agent/src/service/permissions.ts
Propagate agent callbacks and context through chat runs so remote skills and sub-agents stay in sync per request. Update queue and runtime timeout handling so active runs do not time out while waiting behind earlier work.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/extension-agent/src/service/permissions.ts (1)
316-370:⚠️ Potential issue | 🟠 Major未知
source会误走 character 权限分支。参数类型放宽到
string后，这里除了'chatluna'之外都会按 character 处理。只要上游传来一个新值或拼写错误，权限判断就会直接套用characterEnabled、群聊/私聊名单逻辑，而不是显式拒绝或回退到默认语义。💡 建议修改
) { if (source === 'chatluna') { return item.chatlunaEnabled } + + if (source !== 'character') { + return false + } if (!item.characterEnabled) { return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/permissions.ts` around lines 316 - 370, The function isSessionAllowed currently treats any source other than 'chatluna' as a character request, which lets unknown/spelled-wrong sources fall into the character permission branch; update isSessionAllowed to explicitly validate the source (e.g., only accept 'chatluna' and 'character') and return a safe default (reject/false or a documented fallback) for any other values; adjust the early checks so that if source === 'chatluna' returns item.chatlunaEnabled, else if source === 'character' runs the existing character logic (using item.characterEnabled, characterPrivateEnabled, characterGroupEnabled, characterPrivateMode, characterGroupMode, characterPrivateIds, characterGroupIds), and otherwise immediately return false (or the chosen default) to avoid unintentionally applying character permissions to unknown sources.packages/core/src/llm-core/agent/agent.ts (1)
320-331:⚠️ Potential issue | 🟠 Major子代理工具调用丢了
requestId。这里已经继续透传了
subagentContext和callbacks，但没有把父运行里的agentContext.requestId一并带进agent.generate()。runtime_sync.ts里运行态 key 会优先依赖requestId做隔离；一旦这里缺失，同一会话里并发的子代理运行就会退化为共享同一个 conversation 维度，容易互相覆盖或提前清理 runtime catalog。💡 建议修改
const result = await this._agent.generate({ prompt: input.prompt, session: runConfig?.configurable?.session, conversationId: runConfig?.configurable?.conversationId, toolMask: runConfig?.configurable?.toolMask, subagentContext: runConfig?.configurable?.agentContext?.subagentContext, signal: runConfig?.signal, + requestId: + runConfig?.configurable?.agentContext?.requestId, callbacks: runConfig?.callbacks, source: ( runConfig?.configurable as { source?: 'chatluna' | 'character' } )?.source })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/llm-core/agent/agent.ts` around lines 320 - 331, The call to this._agent.generate is missing the parent run's agentContext.requestId, causing runtime isolation issues; fix by passing the requestId into agent.generate—either add requestId: runConfig?.configurable?.agentContext?.requestId to the payload or merge it into the existing subagentContext (e.g., { ...(runConfig?.configurable?.agentContext?.subagentContext), requestId: runConfig?.configurable?.agentContext?.requestId }) so that generate receives the requestId and runtime_sync's isolation works correctly.
🧹 Nitpick comments (3)
packages/core/src/utils/queue.ts (1)
245-258: 清理逻辑中的条件不一致。收集过期项的条件（第 248 行）是
!item.active && now - item.timestamp >= item.timeout，但过滤条件（第 258 行）是item.active || now - item.timestamp < item.timeout。这两个条件在逻辑上是互补的（De Morgan 定律），所以功能正确。但如果将过滤条件改写为
!expired.includes(item)或在收集时直接标记，可以避免重复计算now - item.timestamp，提高可读性和维护性。这是一个小的可选优化建议。
♻️ 可选：简化过滤逻辑
for (const item of this._queue[key]) { - // Cleanup only expires items that are still waiting. Active - // requests are stopped by runtime idle timeout instead. - if (!item.active && now - item.timestamp >= item.timeout) { - expired.push(item) - } + if (item.active || now - item.timestamp < item.timeout) { + continue + } + expired.push(item) } if (expired.length === 0) { return } + const expiredSet = new Set(expired) this._queue[key] = this._queue[key].filter( - (item) => item.active || now - item.timestamp < item.timeout + (item) => !expiredSet.has(item) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/queue.ts` around lines 245 - 258, The cleanup loop in Queue (use this._queue, item.timestamp, item.timeout, item.active) first collects expired items using "!item.active && now - item.timestamp >= item.timeout" then recomputes the same time check in the filter; simplify by marking or collecting expired item identities (e.g., push to an expired array or Set) and change the subsequent filter to exclude items present in that collection (e.g., "!expiredSet.has(item)" or "!expired.includes(item)"), so you avoid recalculating "now - item.timestamp" and make the intent clearer and more maintainable.packages/extension-agent/src/service/sub_agent.ts (1)
370-376: 类型断言可以简化。
runtime.configurable?.agentContext as AgentRunContext | undefined的断言是安全的，但如果runtime.configurable的类型定义中已经包含agentContext?: AgentRunContext，可以省略断言。当前实现不影响功能，仅作为可选的类型改进建议。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/sub_agent.ts` around lines 370 - 376, The call to listRunnableAgents uses an unnecessary type assertion on runtime.configurable?.agentContext; remove the "as AgentRunContext | undefined" cast and pass runtime.configurable?.agentContext directly (retain session and source arguments) so the code relies on the declared type of runtime.configurable.agentContext and avoids redundant casting in the call to listRunnableAgents.packages/extension-agent/src/service/computer.ts (1)
873-899:resolveSessionInput与resolveAgentSessionInput存在逻辑重复。两个方法的核心逻辑几乎相同（查找 subagent、获取 allowedBackends、构建返回对象），区别仅在于输入来源不同。考虑到这是一个"Chill"级别的 review，且两者服务于不同的调用场景（
ChatLunaToolRunnablevsAgentRunContext），当前实现可以接受。如果后续维护中发现同步更新困难，可考虑提取公共逻辑到一个内部辅助方法。
Also applies to: 901-923
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/computer.ts` around lines 873 - 899, The two methods resolveSessionInput and resolveAgentSessionInput duplicate core logic (lookup subagent via this.ctx.chatluna_agent?.subAgent.getCatalogSync(), compute allowedBackends via this.ctx.chatluna_agent?.permission.filterComputerBackends(info, COMPUTER_BACKENDS), and build the return object), so extract that shared logic into a private helper (e.g., buildSessionInputFrom({sub,context,runConfig,backend})) and have both resolveSessionInput (handling ChatLunaToolRunnable) and resolveAgentSessionInput (handling AgentRunContext) call it; ensure the helper accepts the minimal inputs needed (subagent id/context/backends/backend) and returns the same shape currently returned by resolveSessionInput to preserve callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/llm-core/agent/sub-agent.ts`:
- Around line 654-663: When building the sub-agent run context, don't inherit
the parent requestId or callbacks for background runs: change the requestId
field to use options.runConfig?.configurable?.agentContext?.requestId only when
options.background is false (otherwise set it to undefined/new local id), and
similarly set callbacks to undefined when options.background is true instead of
copying options.runConfig?.callbacks; apply the same conditional fix to the
other occurrence that builds the run context (the block around the
requestId/callbacks usage at the second site mentioned).
In `@packages/extension-agent/src/service/runtime_sync.ts`:
- Around line 206-233: collectSyncFiles(...) currently only returns items to
write but never removes local files that were deleted remotely, causing drift;
update runtime_sync.ts to detect and delete local skill/subagent files that are
not present in the collected files list before calling agent.skills.reload() /
agent.subAgent.reload(): after building files (from collectSyncFiles for 'skill'
and 'subagent') compute the set of targetPath entries for each kind, scan the
corresponding local root (getRemoteSkillsRoot() and getSubAgentsRoot /
getSubAgentsRootPath(agent.ctx)) for existing files, delete any local file not
in the collected set (use fs.unlink / rimraf for directories), then proceed with
the current mkdir/writeFile steps and call agent.skills.reload(),
agent.subAgent.reload() and agent.refreshConsoleData() as before.
- Around line 248-269: 当前实现对每个 localRoot
都生成写入任务，导致同一个远端技能文件被写入到所有本地技能根目录。修复方法：不要对所有 localRoots
盲目循环，而是确定该远端文件对应的单一本地根目录后只生成一条写入任务；在 runtime_sync.ts 中修改处理 localRoots 的逻辑（替换当前
for (const localRoot of localRoots) … push(...) 结构），例如：计算或查找目标 root（通过解析 file
的路径/skill id 或使用首选 root，如 localRoots[0]，或引入函数 findLocalRootForRemote(file,
localRoots)）然后只对该 targetRoot 构造 targetPath 并向 files 推入一次包含 kind, targetPath,
content 的对象。
In `@packages/extension-agent/src/skills/catalog.ts`:
- Around line 14-26: The fallback local config lookup currently builds
localByName from the raw scanned skills, so when resolving base for a remote
skill you may pick a shadowed local entry; instead, rebuild the name->skill map
using the shadowing-prioritized list from applyShadowing(skills, preferRemote)
(or otherwise derive the effective local entry from that output) and use that
map when computing base = configItems[skill.id] ?? (skill.remote ?
configItems[localByName.get(skill.name)?.id ?? ''] : undefined); ensure the map
only contains the effective (non-shadowed) local skill per name so
enabled/authority/subAgents come from the same prioritized item used for catalog
generation.
---
Outside diff comments:
In `@packages/core/src/llm-core/agent/agent.ts`:
- Around line 320-331: The call to this._agent.generate is missing the parent
run's agentContext.requestId, causing runtime isolation issues; fix by passing
the requestId into agent.generate—either add requestId:
runConfig?.configurable?.agentContext?.requestId to the payload or merge it into
the existing subagentContext (e.g., {
...(runConfig?.configurable?.agentContext?.subagentContext), requestId:
runConfig?.configurable?.agentContext?.requestId }) so that generate receives
the requestId and runtime_sync's isolation works correctly.
In `@packages/extension-agent/src/service/permissions.ts`:
- Around line 316-370: The function isSessionAllowed currently treats any source
other than 'chatluna' as a character request, which lets unknown/spelled-wrong
sources fall into the character permission branch; update isSessionAllowed to
explicitly validate the source (e.g., only accept 'chatluna' and 'character')
and return a safe default (reject/false or a documented fallback) for any other
values; adjust the early checks so that if source === 'chatluna' returns
item.chatlunaEnabled, else if source === 'character' runs the existing character
logic (using item.characterEnabled, characterPrivateEnabled,
characterGroupEnabled, characterPrivateMode, characterGroupMode,
characterPrivateIds, characterGroupIds), and otherwise immediately return false
(or the chosen default) to avoid unintentionally applying character permissions
to unknown sources.
---
Nitpick comments:
In `@packages/core/src/utils/queue.ts`:
- Around line 245-258: The cleanup loop in Queue (use this._queue,
item.timestamp, item.timeout, item.active) first collects expired items using
"!item.active && now - item.timestamp >= item.timeout" then recomputes the same
time check in the filter; simplify by marking or collecting expired item
identities (e.g., push to an expired array or Set) and change the subsequent
filter to exclude items present in that collection (e.g.,
"!expiredSet.has(item)" or "!expired.includes(item)"), so you avoid
recalculating "now - item.timestamp" and make the intent clearer and more
maintainable.
In `@packages/extension-agent/src/service/computer.ts`:
- Around line 873-899: The two methods resolveSessionInput and
resolveAgentSessionInput duplicate core logic (lookup subagent via
this.ctx.chatluna_agent?.subAgent.getCatalogSync(), compute allowedBackends via
this.ctx.chatluna_agent?.permission.filterComputerBackends(info,
COMPUTER_BACKENDS), and build the return object), so extract that shared logic
into a private helper (e.g.,
buildSessionInputFrom({sub,context,runConfig,backend})) and have both
resolveSessionInput (handling ChatLunaToolRunnable) and resolveAgentSessionInput
(handling AgentRunContext) call it; ensure the helper accepts the minimal inputs
needed (subagent id/context/backends/backend) and returns the same shape
currently returned by resolveSessionInput to preserve callers.
In `@packages/extension-agent/src/service/sub_agent.ts`:
- Around line 370-376: The call to listRunnableAgents uses an unnecessary type
assertion on runtime.configurable?.agentContext; remove the "as AgentRunContext
| undefined" cast and pass runtime.configurable?.agentContext directly (retain
session and source arguments) so the code relies on the declared type of
runtime.configurable.agentContext and avoids redundant casting in the call to
listRunnableAgents.
🪄 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: 09705d4b-eb08-4d3f-aa1d-ea6a3fdd67d1
📒 Files selected for processing (23)
packages/core/src/llm-core/agent/agent.tspackages/core/src/llm-core/agent/executor.tspackages/core/src/llm-core/agent/legacy-executor.tspackages/core/src/llm-core/agent/sub-agent.tspackages/core/src/llm-core/agent/types.tspackages/core/src/llm-core/chain/agent_chat_chain.tspackages/core/src/llm-core/chain/base.tspackages/core/src/llm-core/chain/chat_chain.tspackages/core/src/llm-core/chat/default.tspackages/core/src/llm-core/platform/types.tspackages/core/src/services/chat.tspackages/core/src/services/conversation_runtime.tspackages/core/src/services/types.tspackages/core/src/utils/queue.tspackages/extension-agent/resources/skills/agent-config-admin/SKILL.mdpackages/extension-agent/src/service/computer.tspackages/extension-agent/src/service/index.tspackages/extension-agent/src/service/permissions.tspackages/extension-agent/src/service/runtime_sync.tspackages/extension-agent/src/service/skills.tspackages/extension-agent/src/service/sub_agent.tspackages/extension-agent/src/skills/catalog.tspackages/extension-agent/src/utils/shadow.ts
✅ Files skipped from review due to trivial changes (1)
- packages/extension-agent/resources/skills/agent-config-admin/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/services/conversation_runtime.ts
- packages/extension-agent/src/service/skills.ts
Drop per-run remote catalog state in favor of syncing files back through the shared agent service and background task handling. Move common extension tools onto shared tool wrappers and output truncation so request and command flows keep consistent runtime behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/extension-tools/src/plugins/think.ts (2)
95-101: 修复格式问题。静态分析工具标记了第 98-99 行的格式问题（多余的换行和空格）。
🔧 建议的格式修复
const indices = parts .map((p) => parseInt(p)) - .filter( - (n) => - !isNaN(n) && n >= 1 && n <= options.length - ) + .filter((n) => !isNaN(n) && n >= 1 && n <= options.length)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/think.ts` around lines 95 - 101, The block computing indices has extra blank lines/newlines around the .filter call causing a formatting lint error; inline and tidy the chaining on the parts -> parseInt -> filter sequence so it reads compactly (e.g., chain .map((p) => parseInt(p)).filter((n) => !isNaN(n) && n >= 1 && n <= options.length)), keeping references to parts, parseInt, filter, n and options.length intact to preserve behavior.
145-152: 修复 schema 描述的格式问题。静态分析工具标记了第 150 行的格式问题，建议将长字符串拆分为多行。
🔧 建议的格式修复
{ name: 'user_confirm', description: USER_CONFIRM_DESCRIPTION, - schema: z - .string() - .describe('The open-ended prompt or confirmation message sent to the user.') + schema: z.string().describe( + 'The open-ended prompt or confirmation message sent to the user.' + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/think.ts` around lines 145 - 152, The schema description for the 'user_confirm' field (z.string().describe(...)) uses a long single-line string (USER_CONFIRM_DESCRIPTION) that the linter flags; update the description to be split across multiple lines by turning USER_CONFIRM_DESCRIPTION into a multiline template literal or by concatenating shorter string fragments so the describe call remains z.string().describe(USER_CONFIRM_DESCRIPTION) but the constant itself is wrapped/line-broken across multiple lines to satisfy the formatter and static analysis.packages/extension-agent/src/service/skills.ts (1)
495-505: 修复格式问题并考虑简化类型提取。静态分析工具正确标记了第 504-505 行的格式问题。此外，这里的类型提取过于复杂，可以直接使用类型断言简化。
♻️ 建议的修复
const sub = - ( - runtime.configurable?.agentContext as { - subagentContext?: ChatLunaToolRunnable['configurable']['agentContext'] extends infer T - ? T extends { subagentContext?: infer U } - ? U - : never - : never - } - )?.subagentContext ?? - runtime.configurable?.subagentContext + (runtime.configurable?.agentContext as { subagentContext?: unknown }) + ?.subagentContext ?? runtime.configurable?.subagentContext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/skills.ts` around lines 495 - 505, 当前对 sub 的赋值表达式格式混乱且使用了冗长的条件类型提取；将该表达式格式化为单行或更清晰的多行并用简单的类型断言替代复杂的 infer 嵌套。定位到变量 sub 的赋值处（读取 runtime.configurable 和可能的 subagentContext），将类型推断改为比如将 runtime.configurable?.agentContext 断言为所需的类型或直接访问 runtime.configurable?.subagentContext，保持可读性并移除复杂的 ChatLunaToolRunnable 中的 infer 嵌套表达式以修复静态分析器的格式/类型警告。packages/extension-tools/src/plugins/command.ts (1)
75-95: 修复格式问题。静态分析工具标记了第 75 行的格式问题，建议将条件拆分为多行。
🔧 建议的格式修复
- if (!config.commandAutoExecute && (matchedCommand?.confirm ?? true)) { + if ( + !config.commandAutoExecute && + (matchedCommand?.confirm ?? true) + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/command.ts` around lines 75 - 95, The if-statement with the compound condition using config.commandAutoExecute and matchedCommand?.confirm should be reformatted across multiple lines for readability and to satisfy the linter; update the conditional in the block that checks config.commandAutoExecute && (matchedCommand?.confirm ?? true) so each operand is on its own line and the whole condition is wrapped in parentheses, preserving the existing logic and behavior in the surrounding code that generates validationString and calls session.send and session.prompt.packages/extension-agent/src/service/runtime_sync.ts (1)
56-98: 避免this别名，使用箭头函数绑定。ESLint 标记了第 58 行的
const svc = this模式。可以通过将回调方法提取为绑定的箭头函数来解决。♻️ 建议的重构
private createProvider(): ChatCallbacksProvider { - return async () => { - const svc = this + const registerRun = this.registerRun.bind(this) + const finishRun = this.finishRun.bind(this) + return async () => { return CallbackManager.fromHandlers({ async handleChainStart( _chain, _inputs, runId, _parentRunId, _tags, metadata ) { if (!runId) { return } const context = (metadata?.chatlunaAgent ?? metadata?.['chatlunaAgent']) as | AgentRunContext | undefined if (!context) { return } - svc.registerRun(String(runId), context) + registerRun(String(runId), context) }, async handleChainEnd(_output, runId) { if (!runId) { return } - await svc.finishRun(String(runId)) + await finishRun(String(runId)) }, async handleChainError(_err, runId) { if (!runId) { return } - await svc.finishRun(String(runId)) + await finishRun(String(runId)) } }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/runtime_sync.ts` around lines 56 - 98, The code uses a `const svc = this` alias inside createProvider to access instance methods; replace that pattern by using arrow function callbacks so `this` is lexically bound instead of aliased. In createProvider (and the CallbackManager.fromHandlers block) change handleChainStart, handleChainEnd and handleChainError to arrow functions that call this.registerRun and this.finishRun directly (e.g., handleChainStart: async (...) => { ... this.registerRun(String(runId), context) } ), remove the `const svc = this` line, and ensure runId checks and metadata extraction remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/llm-core/agent/sub-agent.ts`:
- Around line 654-657: The ternary branch assigning requestId contains redundant
parentheses that trigger a prettier warning; in the requestId assignment inside
sub-agent.ts (the expression using options.input.background, runId and
options.runConfig?.configurable?.agentContext?.requestId) remove the unnecessary
parentheses around the right-hand conditional so it becomes: requestId:
options.input.background ? runId :
options.runConfig?.configurable?.agentContext?.requestId ?? runId, ensuring the
identifiers requestId, options.input.background, runId and
options.runConfig?.configurable?.agentContext?.requestId are used as shown.
In `@packages/extension-tools/src/plugins/request.ts`:
- Around line 156-161: The web_post tool is using WEBFETCH_DESCRIPTION (a
GET-oriented description); create a POST-specific description constant (e.g.,
WEBPOST_DESCRIPTION) that explains sending a JSON payload in a POST request and
returning the response, then replace WEBFETCH_DESCRIPTION with this new constant
in the tool definition for the object with name 'web_post' (ensure the new
constant is used where the web_post tool is constructed alongside
webPostSchema).
---
Nitpick comments:
In `@packages/extension-agent/src/service/runtime_sync.ts`:
- Around line 56-98: The code uses a `const svc = this` alias inside
createProvider to access instance methods; replace that pattern by using arrow
function callbacks so `this` is lexically bound instead of aliased. In
createProvider (and the CallbackManager.fromHandlers block) change
handleChainStart, handleChainEnd and handleChainError to arrow functions that
call this.registerRun and this.finishRun directly (e.g., handleChainStart: async
(...) => { ... this.registerRun(String(runId), context) } ), remove the `const
svc = this` line, and ensure runId checks and metadata extraction remain
unchanged.
In `@packages/extension-agent/src/service/skills.ts`:
- Around line 495-505: 当前对 sub
的赋值表达式格式混乱且使用了冗长的条件类型提取；将该表达式格式化为单行或更清晰的多行并用简单的类型断言替代复杂的 infer 嵌套。定位到变量 sub
的赋值处（读取 runtime.configurable 和可能的 subagentContext），将类型推断改为比如将
runtime.configurable?.agentContext 断言为所需的类型或直接访问
runtime.configurable?.subagentContext，保持可读性并移除复杂的 ChatLunaToolRunnable 中的 infer
嵌套表达式以修复静态分析器的格式/类型警告。
In `@packages/extension-tools/src/plugins/command.ts`:
- Around line 75-95: The if-statement with the compound condition using
config.commandAutoExecute and matchedCommand?.confirm should be reformatted
across multiple lines for readability and to satisfy the linter; update the
conditional in the block that checks config.commandAutoExecute &&
(matchedCommand?.confirm ?? true) so each operand is on its own line and the
whole condition is wrapped in parentheses, preserving the existing logic and
behavior in the surrounding code that generates validationString and calls
session.send and session.prompt.
In `@packages/extension-tools/src/plugins/think.ts`:
- Around line 95-101: The block computing indices has extra blank lines/newlines
around the .filter call causing a formatting lint error; inline and tidy the
chaining on the parts -> parseInt -> filter sequence so it reads compactly
(e.g., chain .map((p) => parseInt(p)).filter((n) => !isNaN(n) && n >= 1 && n <=
options.length)), keeping references to parts, parseInt, filter, n and
options.length intact to preserve behavior.
- Around line 145-152: The schema description for the 'user_confirm' field
(z.string().describe(...)) uses a long single-line string
(USER_CONFIRM_DESCRIPTION) that the linter flags; update the description to be
split across multiple lines by turning USER_CONFIRM_DESCRIPTION into a multiline
template literal or by concatenating shorter string fragments so the describe
call remains z.string().describe(USER_CONFIRM_DESCRIPTION) but the constant
itself is wrapped/line-broken across multiple lines to satisfy the formatter and
static analysis.
🪄 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: c6cb1c53-c40f-48ae-9cdd-2e4a816848f2
⛔ Files ignored due to path filters (1)
packages/extension-tools/package.jsonis excluded by!**/*.json
📒 Files selected for processing (17)
packages/core/src/llm-core/agent/sub-agent.tspackages/extension-agent/src/computer/tools/base.tspackages/extension-agent/src/service/computer.tspackages/extension-agent/src/service/index.tspackages/extension-agent/src/service/runtime_sync.tspackages/extension-agent/src/service/skills.tspackages/extension-agent/src/service/sub_agent.tspackages/extension-agent/src/skills/scan.tspackages/extension-agent/src/sub-agent/scan.tspackages/extension-agent/src/types.tspackages/extension-tools/src/config.tspackages/extension-tools/src/plugins/command.tspackages/extension-tools/src/plugins/group.tspackages/extension-tools/src/plugins/music.tspackages/extension-tools/src/plugins/request.tspackages/extension-tools/src/plugins/think.tspackages/extension-tools/src/plugins/todos.ts
💤 Files with no reviewable changes (1)
- packages/extension-agent/src/sub-agent/scan.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-tools/src/config.ts
- packages/extension-agent/src/service/index.ts
Give the POST request tool its own prompt text so it describes JSON submission behavior instead of the read-only fetch flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/extension-tools/src/plugins/request.ts (2)
63-65:web_post的 URL 描述与实际用途不符。
webPostSchema中url字段的描述是'The URL to fetch content from'，这是从 GET 工具复制过来的。对于 POST 工具，描述应该更准确地反映其用途。♻️ 建议的修复
const webPostSchema = z.object({ - url: z.string().describe('The URL to fetch content from'), + url: z.string().describe('The URL to send the POST request to'), data: z.record(z.any()).describe('The JSON payload to send'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/request.ts` around lines 63 - 65, The url field in webPostSchema currently has a GET-oriented description; update the z.object for webPostSchema so the url property's description reflects POST semantics (e.g., "The URL to send the POST request to" or "The endpoint to which the JSON payload will be posted") so it accurately documents the POST tool; locate the url property on webPostSchema and replace the description string accordingly.
27-27: 工具描述行超过最大长度限制。静态分析工具标记第 27 行（188 字符）和第 41 行（187 字符）超过了 160 字符的最大行长限制。建议将这些行拆分。
♻️ 建议的修复
Usage notes: - - IMPORTANT: if another tool is present that offers better web fetching capabilities, is more targeted to the task, or has fewer restrictions, prefer using that tool instead of this one. + - IMPORTANT: if another tool is present that offers better web fetching + capabilities, is more targeted to the task, or has fewer restrictions, + prefer using that tool instead of this one. - The URL must be a fully-formed valid URLUsage notes: - - IMPORTANT: if another tool is present that offers better web request capabilities, is more targeted to the task, or has fewer restrictions, prefer using that tool instead of this one. + - IMPORTANT: if another tool is present that offers better web request + capabilities, is more targeted to the task, or has fewer restrictions, + prefer using that tool instead of this one. - The URL must be a fully-formed valid URLAlso applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/request.ts` at line 27, A long comment line starting with "IMPORTANT: if another tool is present that offers better web fetching capabilities..." exceeds the 160-char line-length limit (occurrences at the comment beginning and the similar one later). Fix by breaking that comment into multiple shorter comment lines or sentences under 160 characters each (e.g., split after a clause such as "prefer using that tool instead of this one."), and apply the same split to the repeated occurrence (the second long comment near the 41st line) so both instances comply with the line-length rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-tools/src/plugins/request.ts`:
- Around line 351-357: The fallback branch that writes the large output to disk
(using join, mkdir, writeFile, randomUUID) lacks error handling and can throw,
so wrap the mkdir/writeFile sequence in a try/catch; on success keep returning
the full "Output too large" message with the saved file path, and on error log
the failure and return the truncated preview exactly as
ChatLunaAgentService.truncateTextOutput does (i.e., include the truncated text
but omit or replace the file path), ensuring the function still returns a safe
truncated preview instead of letting the exception bubble up.
---
Nitpick comments:
In `@packages/extension-tools/src/plugins/request.ts`:
- Around line 63-65: The url field in webPostSchema currently has a GET-oriented
description; update the z.object for webPostSchema so the url property's
description reflects POST semantics (e.g., "The URL to send the POST request to"
or "The endpoint to which the JSON payload will be posted") so it accurately
documents the POST tool; locate the url property on webPostSchema and replace
the description string accordingly.
- Line 27: A long comment line starting with "IMPORTANT: if another tool is
present that offers better web fetching capabilities..." exceeds the 160-char
line-length limit (occurrences at the comment beginning and the similar one
later). Fix by breaking that comment into multiple shorter comment lines or
sentences under 160 characters each (e.g., split after a clause such as "prefer
using that tool instead of this one."), and apply the same split to the repeated
occurrence (the second long comment near the 41st line) so both instances comply
with the line-length rule.
🪄 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: 2603bf33-e4bb-47c1-92f6-2eec52c91f2e
📒 Files selected for processing (1)
packages/extension-tools/src/plugins/request.ts
Handle file write errors when persisting oversized request responses so the tool still returns a truncated preview instead of failing the whole request flow.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/extension-tools/src/plugins/request.ts (1)
351-357:⚠️ Potential issue | 🟠 Major回退写盘分支仍然会把工具调用打崩。
这里一旦
mkdir或writeFile失败，整个web_fetch/web_post会直接抛错，而不是像packages/extension-agent/src/service/index.ts:584-648那样记录日志并返回安全的截断预览。既然上面已经把主路径统一到了truncateTextOutput()，这个回退分支也应该保持同样的语义。💡 建议修复
const filePath = join(dir, `${name}-${Date.now()}-${randomUUID()}.txt`) - await mkdir(dir, { recursive: true }) - await writeFile(filePath, text, 'utf-8') - return `Output too large (${text.length} chars). Truncated preview below. + try { + await mkdir(dir, { recursive: true }) + await writeFile(filePath, text, 'utf-8') + return `Output too large (${text.length} chars). Truncated preview below. Full output saved to: ${filePath} +Use file_read with this path plus offset/limit to inspect more. ${text.slice(0, limit)}\n...[output truncated]` + } catch (e) { + ctx.logger.error(e) + return `Output too large (${text.length} chars). Truncated preview below. +Failed to save full output. + +${text.slice(0, limit)}\n...[output truncated]` + }Based on learnings "Applies to **/*.ts : Use try/catch around LLM calls and file I/O; log via
logger.error(e)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/request.ts` around lines 351 - 357, The fallback branch that writes to disk (creating filePath with join + Date.now()+randomUUID and calling mkdir/writeFile) must not let I/O errors bubble up; wrap the mkdir and writeFile calls in a try/catch, and on catch call logger.error(e) and then return the same truncated-preview string (the one using text.slice(0, limit)) so web_fetch/web_post keep the safe truncated behaviour; ensure you reference the same truncateTextOutput semantics (use truncateTextOutput or replicate its return format) and avoid rethrowing the error.
🧹 Nitpick comments (1)
packages/extension-tools/src/plugins/request.ts (1)
268-279: 把normalizeUrl()内联掉。仓库规则明确禁止新增
normalizeXXXhelper，而且它只有一个调用点。把这几行直接放回requestUrl()会更符合当前文件风格，也少一层跳转。As per coding guidelines "Do NOT create resolveXXX, normalizeXXX, ensureXXX, toSafeXXX functions—these are banned patterns"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/request.ts` around lines 268 - 279, The helper function normalizeUrl violates the "no normalizeXXX helpers" rule and has a single call site; inline its logic into requestUrl: replace the call to normalizeUrl(url) inside requestUrl with the helper's body (trim the input, convert "http://" to "https://" by slicing 7 chars, and throw the same Error if the trimmed URL doesn't start with "https://"), then remove the normalizeUrl function definition entirely; keep existing error text and preserve behavior (trimming, http->https conversion, and validation) and update only the call site and the removed helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/extension-tools/src/plugins/request.ts`:
- Around line 351-357: The fallback branch that writes to disk (creating
filePath with join + Date.now()+randomUUID and calling mkdir/writeFile) must not
let I/O errors bubble up; wrap the mkdir and writeFile calls in a try/catch, and
on catch call logger.error(e) and then return the same truncated-preview string
(the one using text.slice(0, limit)) so web_fetch/web_post keep the safe
truncated behaviour; ensure you reference the same truncateTextOutput semantics
(use truncateTextOutput or replicate its return format) and avoid rethrowing the
error.
---
Nitpick comments:
In `@packages/extension-tools/src/plugins/request.ts`:
- Around line 268-279: The helper function normalizeUrl violates the "no
normalizeXXX helpers" rule and has a single call site; inline its logic into
requestUrl: replace the call to normalizeUrl(url) inside requestUrl with the
helper's body (trim the input, convert "http://" to "https://" by slicing 7
chars, and throw the same Error if the trimmed URL doesn't start with
"https://"), then remove the normalizeUrl function definition entirely; keep
existing error text and preserve behavior (trimming, http->https conversion, and
validation) and update only the call site and the removed helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7b947f1e-e794-41ce-9a56-d5a78ac63662
📒 Files selected for processing (1)
packages/extension-tools/src/plugins/request.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/extension-tools/src/plugins/request.ts (1)
268-279: 把normalizeUrl()内联回requestUrl()。这个 helper 只有一个调用点，而且函数体很短；继续单独抽出来会让这段流程比实际更绕。更重要的是，
normalizeXXX在仓库规则里是明确禁用的命名模式。As per coding guidelines "Do NOT createresolveXXX,normalizeXXX,ensureXXX,toSafeXXXfunctions—these are banned patterns" and "Do NOT create extra functions for short logic; if a function body would be 1-5 lines, inline it at the call site instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/request.ts` around lines 268 - 279, Inline the normalizeUrl logic into requestUrl and remove the separate normalizeUrl function: locate the call site in requestUrl, replace it with the trimmed/validation/https-conversion logic currently in normalizeUrl (trim the input, if it starts with 'http://' replace with 'https://' using trimmed.slice(7), otherwise ensure it starts with 'https://' or throw the same Error message), then delete the now-unused normalizeUrl function so the URL normalization exists only inside requestUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-tools/src/plugins/request.ts`:
- Around line 235-256: The code currently awaits res.text() (in the plugin.fetch
response handling) which can load a huge body into memory; instead, first check
res.headers.get('content-length') and reject early if it exceeds
requestMaxOutputLength, and otherwise consume res.body as a byte stream (using
the response ReadableStream/getReader or equivalent) accumulating bytes up to
requestMaxOutputLength; if the limit is exceeded, call controller.abort() and
throw a bounded-error; only after streaming and enforcing the byte-length cap
should you call convertContent and formatOutput with the collected (or
truncated) text. Ensure you reference plugin.fetch response handling,
controller, requestMaxOutputLength, convertContent, and formatOutput when making
changes.
---
Nitpick comments:
In `@packages/extension-tools/src/plugins/request.ts`:
- Around line 268-279: Inline the normalizeUrl logic into requestUrl and remove
the separate normalizeUrl function: locate the call site in requestUrl, replace
it with the trimmed/validation/https-conversion logic currently in normalizeUrl
(trim the input, if it starts with 'http://' replace with 'https://' using
trimmed.slice(7), otherwise ensure it starts with 'https://' or throw the same
Error message), then delete the now-unused normalizeUrl function so the URL
normalization exists only inside requestUrl.
🪄 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: 16570c99-b64b-418d-af49-b05cc8ec7f1c
📒 Files selected for processing (1)
packages/extension-tools/src/plugins/request.ts
Stop reading web request bodies once they exceed the configured output limit so large responses fail early instead of being fully buffered before truncation logic runs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-tools/src/plugins/request.ts`:
- Around line 64-65: Update the zod schema description for the web_post tool's
url field to reflect POST semantics: replace the current description string "The
URL to fetch content from" with something indicating sending a JSON payload
(e.g., "The URL to send JSON payload to" or similar) so the url description
matches the tool behavior; locate the url field in the request plugin schema
(the z.string().describe call alongside data: z.record(...)) and edit only the
description text.
- Line 27: The long usage note string "IMPORTANT: if another tool is present
that offers better web fetching capabilities, is more targeted to the task, or
has fewer restrictions, prefer using that tool instead of this one." (and the
similar long note at the other occurrence) exceeds the 160-char line width;
split each into two shorter bullet lines so each line <160 chars to satisfy lint
rules—locate and edit the two occurrences in
packages/extension-tools/src/plugins/request.ts (the long usage note at the
shown diff and the similar one around line 41) and refactor them into two bullet
sentences or two separate lines with preserved meaning and punctuation.
- Around line 314-325: The new helper function normalizeUrl violates the naming
ban; remove normalizeUrl and inline its logic into requestUrl: trim the input,
if it starts with 'http://' replace the scheme with 'https://' by slicing off 7
chars and prefixing 'https://', if it does not start with 'https://' throw the
same Error('URL must be a fully-formed HTTP/HTTPS URL'), and otherwise use the
trimmed value; ensure requestUrl uses this exact behavior and error message so
functionality remains unchanged and delete the normalizeUrl symbol.
🪄 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: 7c3261e2-563a-461c-998f-ad60243aeab4
📒 Files selected for processing (1)
packages/extension-tools/src/plugins/request.ts
Apply the current lint-driven cleanup across the in-flight agent and extension tool changes so the branch stays committable after the latest request tool updates.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/extension-tools/src/plugins/command.ts (1)
267-274:⚠️ Potential issue | 🟠 Major重命名目录后没有清理旧的
koishi_command_skills。现在只删/写
koishi-command-skills。升级后旧目录会继续留在data/chatluna/skills下，技能发现会同时看到新旧两份目录，产生重复或过期的 skill 文档。建议修复
async function removeCommandSkill(ctx: Context) { - await rm( - join(ctx.baseDir, 'data/chatluna/skills', 'koishi-command-skills'), - { - recursive: true, - force: true - } - ) + await Promise.all([ + rm( + join(ctx.baseDir, 'data/chatluna/skills', 'koishi-command-skills'), + { + recursive: true, + force: true + } + ), + rm( + join(ctx.baseDir, 'data/chatluna/skills', 'koishi_command_skills'), + { + recursive: true, + force: true + } + ) + ]) } @@ - await rm(skillDir, { recursive: true, force: true }) + await Promise.all([ + rm(skillDir, { recursive: true, force: true }), + rm( + join(ctx.baseDir, 'data/chatluna/skills', 'koishi_command_skills'), + { recursive: true, force: true } + ) + ])Also applies to: 283-309, 319-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/command.ts` around lines 267 - 274, The cleanup currently only removes the new hyphenated directory 'koishi-command-skills' in removeCommandSkill; update the cleanup logic to also remove the old underscore-named directory 'koishi_command_skills' (and mirror this change in the other cleanup blocks referenced around the same code paths) by calling rm for both join(ctx.baseDir, 'data/chatluna/skills', 'koishi-command-skills') and join(ctx.baseDir, 'data/chatluna/skills', 'koishi_command_skills') (or the equivalent cleanup function used elsewhere) so both legacy and new directories are deleted and duplicate/stale skill documents are not left behind.packages/extension-tools/src/plugins/think.ts (1)
40-52:⚠️ Potential issue | 🟠 Major空
options输入会在默认分支直接崩溃。
questionSchema允许options: []，但 Line 89 会直接读取options[0].label。只要模型传入空数组并且用户回车，这个工具就会抛异常。建议修复
const QuestionItemSchema = z.object({ question: z.string().describe('Complete question'), header: z.string().describe('Very short label (max 30 chars)'), - options: z.array(QuestionOptionSchema).describe('Available choices'), + options: z + .array(QuestionOptionSchema) + .min(1) + .describe('Available choices'), multiple: z .boolean() .optional() .describe('Allow selecting multiple choices') })Also applies to: 87-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-tools/src/plugins/think.ts` around lines 40 - 52, The schema permits an empty options array but the consumer reads options[0].label (causing a crash); update QuestionItemSchema to require at least one option (e.g., make options: z.array(QuestionOptionSchema).min(1)) and/or add a runtime guard where options[0].label is accessed (ensure options.length > 0 before reading and provide a safe fallback or throw a validation error); reference QuestionItemSchema, questionSchema, and the places that read options[0].label to implement the schema change and/or runtime check.
♻️ Duplicate comments (2)
packages/extension-agent/src/service/runtime_sync.ts (2)
169-196:⚠️ Potential issue | 🟠 Major远端删除不会回收本地旧文件。
当前流程只会写入
collectSyncFiles()返回的文件，没有在reload()前清理“远端已删、本地仍在”的 skill/subagent。这样远端一旦删掉 runtime 文件，本地扫描结果就会持续带着陈旧项。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/runtime_sync.ts` around lines 169 - 196, The sync process writes only files returned by collectSyncFiles (used for 'skill' and 'subagent') but never deletes local files that were removed remotely; before calling agent.skills.reload() / agent.subAgent.reload(), compute the set of current local runtime files under the same target roots and remove any local files whose paths are not present in the collected files array (i.e., delete files that used to exist locally but are missing from collectSyncFiles results); perform deletion for both kinds (use item.kind === 'skill' and item.kind === 'subagent' to partition), run removals before calling reload(), and keep the mkdir/writeFile flow unchanged for adding/updating files, then call agent.refreshConsoleData() as before.
211-231:⚠️ Potential issue | 🟠 Major同一个远端 skill 会被写进所有本地 skill 根目录。
这里对每个
localRoot都会追加一条写入任务；只要配置了多个技能目录，一个远端文件就会在所有目录里各落一份。reload()后即使暂时靠覆盖顺序“看起来可用”，后续解析和编辑目标都会变得不稳定。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/service/runtime_sync.ts` around lines 211 - 231, The loop over localRoots in runtime_sync.ts is adding a write task for every localRoot so a single remote skill file is duplicated across all local skill roots; instead determine the single correct local root before pushing to files (e.g., compute the matching localRoot using the skill identifier or path convention, or use a mapping from kind/skillId to its localRoot) and only create one { kind, targetPath, content } entry for that chosen root; replace the current per-localRoot push logic (the for (const localRoot of localRoots) / targetPath / files.push block) with logic that selects one targetPath and pushes once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-agent/src/service/runtime_sync.ts`:
- Around line 138-143: The code is swallowing errors from
agent.computer.getAgentSession(state.context) via .catch(() => undefined);
remove that catch and instead await getAgentSession inside a try/catch that logs
the error and rethrows (or simply let it propagate) so failures are visible and
the outer error path can handle them; update the block around
agent.computer.getAgentSession(...) and syncRuntimeSession(agent, session) to
either (a) await getAgentSession(...) directly and allow exceptions to bubble,
or (b) wrap it in try { const session = await
agent.computer.getAgentSession(state.context); if (session && session.backend
!== 'local') await syncRuntimeSession(agent, session); } catch (err) {
logger.error('getAgentSession failed', err); throw err; } so errors are not
silently swallowed.
In `@packages/extension-tools/src/plugins/command.ts`:
- Around line 51-77: The code currently only uses matchedCommand for
confirmation but always calls session.execute(command, true), allowing unlisted
or blacklisted commands to run; update the flow around
commandList/matchedCommand in the handler (the block using commandList,
matchedCommand, commandAutoExecute and runConfig.configurable.session) to
require a valid matchedCommand and to check commandBlacklist before calling
session.execute(command, true)—if no match or the command is blacklisted,
prevent execution (return or surface an error/confirmation), and apply the same
change to the other occurrence in the file (the similar block around lines
handling commands 100-145).
In `@packages/extension-tools/src/plugins/think.ts`:
- Around line 22-33: The USER_CONFIRM_DESCRIPTION constant for the user_confirm
tool references a non-existent "built_question" tool; update the description to
reference the actual registered tool name "question" (and similarly fix the
duplicate occurrence around the 126-146 block) so the guidance points to
"question" when advising to use the other tool for specific-option selection;
locate USER_CONFIRM_DESCRIPTION and any other strings in this file that mention
"built_question" and replace them with "question".
---
Outside diff comments:
In `@packages/extension-tools/src/plugins/command.ts`:
- Around line 267-274: The cleanup currently only removes the new hyphenated
directory 'koishi-command-skills' in removeCommandSkill; update the cleanup
logic to also remove the old underscore-named directory 'koishi_command_skills'
(and mirror this change in the other cleanup blocks referenced around the same
code paths) by calling rm for both join(ctx.baseDir, 'data/chatluna/skills',
'koishi-command-skills') and join(ctx.baseDir, 'data/chatluna/skills',
'koishi_command_skills') (or the equivalent cleanup function used elsewhere) so
both legacy and new directories are deleted and duplicate/stale skill documents
are not left behind.
In `@packages/extension-tools/src/plugins/think.ts`:
- Around line 40-52: The schema permits an empty options array but the consumer
reads options[0].label (causing a crash); update QuestionItemSchema to require
at least one option (e.g., make options: z.array(QuestionOptionSchema).min(1))
and/or add a runtime guard where options[0].label is accessed (ensure
options.length > 0 before reading and provide a safe fallback or throw a
validation error); reference QuestionItemSchema, questionSchema, and the places
that read options[0].label to implement the schema change and/or runtime check.
---
Duplicate comments:
In `@packages/extension-agent/src/service/runtime_sync.ts`:
- Around line 169-196: The sync process writes only files returned by
collectSyncFiles (used for 'skill' and 'subagent') but never deletes local files
that were removed remotely; before calling agent.skills.reload() /
agent.subAgent.reload(), compute the set of current local runtime files under
the same target roots and remove any local files whose paths are not present in
the collected files array (i.e., delete files that used to exist locally but are
missing from collectSyncFiles results); perform deletion for both kinds (use
item.kind === 'skill' and item.kind === 'subagent' to partition), run removals
before calling reload(), and keep the mkdir/writeFile flow unchanged for
adding/updating files, then call agent.refreshConsoleData() as before.
- Around line 211-231: The loop over localRoots in runtime_sync.ts is adding a
write task for every localRoot so a single remote skill file is duplicated
across all local skill roots; instead determine the single correct local root
before pushing to files (e.g., compute the matching localRoot using the skill
identifier or path convention, or use a mapping from kind/skillId to its
localRoot) and only create one { kind, targetPath, content } entry for that
chosen root; replace the current per-localRoot push logic (the for (const
localRoot of localRoots) / targetPath / files.push block) with logic that
selects one targetPath and pushes once.
🪄 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: 90f448ca-2d70-49ef-beec-6de57966127f
📒 Files selected for processing (8)
packages/extension-agent/src/service/runtime_sync.tspackages/extension-agent/src/service/skills.tspackages/extension-agent/src/sub-agent/scan.tspackages/extension-tools/src/plugins/command.tspackages/extension-tools/src/plugins/music.tspackages/extension-tools/src/plugins/request.tspackages/extension-tools/src/plugins/think.tspackages/extension-tools/src/plugins/todos.ts
💤 Files with no reviewable changes (1)
- packages/extension-agent/src/sub-agent/scan.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-agent/src/service/skills.ts
- packages/extension-tools/src/plugins/music.ts
Use the shadowed skill view when inheriting local config for remote skills, and reject blocked or unknown command tool calls before execution. Update the interaction tool copy to reference the current question tool name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-tools/src/plugins/command.ts`:
- Around line 52-78: The current lookup using commandList.find(...) with prefix
checks can return a parent or alias match that is too broad; update the
selection logic (the matchedCommand resolution) to pick the most specific
command among all matches (e.g., longest matching name or highest specificity)
instead of the first match, and then perform blacklist checking against the
resolved/parsed command identity (not just baseCommandName) using
config.commandBlacklist so aliases or parent commands cannot bypass blocking;
adjust logic around matchedCommand, baseCommandName, commandList and
commandBlacklist (and where session is used) so the blacklist decision uses the
final resolved command name/parse result.
- Around line 277-279: removeCommandSkill currently only removes the new
directory name 'koishi-command-skills' and misses the historical
'koishi_command_skills', so old skills may remain after upgrade; update the
cleanup to attempt removing both directories (join(ctx.baseDir,
'data/chatluna/skills', 'koishi-command-skills') and join(ctx.baseDir,
'data/chatluna/skills', 'koishi_command_skills')) and apply the same dual-path
removal to the other cleanup calls referenced around the same area (the other
rm/join usages at lines ~293-297 and ~305-306), ensuring you ignore "not found"
errors (or catch and swallow ENOENT) so removal is idempotent.
🪄 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: 500b2cd6-91fa-462b-9e9a-cacdf5f6ff35
📒 Files selected for processing (3)
packages/extension-agent/src/skills/catalog.tspackages/extension-tools/src/plugins/command.tspackages/extension-tools/src/plugins/think.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-agent/src/skills/catalog.ts
- packages/extension-tools/src/plugins/think.ts
Match command tool calls against the most specific command or alias before blacklist checks so nested commands are validated correctly. Also remove both legacy and current command skill directories when refreshing generated command skills.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extension-tools/src/plugins/command.ts`:
- Around line 52-72: The filter is currently allowing ancestor commands when
only a descendant is listed because of the clause
item.startsWith(baseCommandName + '.'); update the matching logic in the flatMap
over commandList so it only permits exact matches or when the requested command
(baseCommandName) is a descendant of an allowed item (keep item ===
baseCommandName and baseCommandName.startsWith(item + '.')) and remove the
item.startsWith(baseCommandName + '.') check; adjust the match selection that
produces matched (and respects the existing sort by exactness and length) so
ancestors aren't authorized just because a subcommand is listed.
🪄 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: 3f53a9f5-5b39-457e-8214-47f1b14cb627
📒 Files selected for processing (1)
packages/extension-tools/src/plugins/command.ts
Stop resolving command tool calls by matching child command names from a parent prefix alone, so only exact names or real subcommand prefixes are accepted during command lookup.
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 tightens agent-related behavior on the better-agent-system branch, normalizes conversation lifecycle hook names, and removes stale tool integrations that no longer match the current runtime.
New Features
Bug fixes
Other Changes