[Refactor] simplify agent extension#881
Merged
Merged
Conversation
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 (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
<review_stack_artifact_start> Walkthrough改动为一次大规模等价重构：将许多辅助函数内联、能力声明统一为类内数组、规范临时目录与环境注入、重写技能扫描/导入/子代理解析与权限合并逻辑，并删除或合并多个模块级常量与小型辅助函数，保留大部分对外签名兼容性。 Changes单一变更集合
Estimated code review effort: 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 and simplifies various components of the extension agent, including backend sessions, sub-agent parsing, catalog building, and path utilities. The review feedback highlights several critical type safety and runtime robustness issues: a potential TypeScript compilation error under strictNullChecks in store.ts due to un-narrowed map lookups, a potential runtime TypeError in run.ts from accessing the computer service without optional chaining, an unsafe property access on a potentially null error object in agentcli_sync.ts, and a type mismatch in e2b.ts where getDesktopInfo returns null instead of the declared undefined type.
![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: ffebac582d
ℹ️ 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: 6
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/sub-agent/parse.ts (1)
374-397:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
readNames的兼容映射作用域过宽，可能错误改写 skill/mcp 名称Line 374-397 对所有名称都执行
mapCompatToolName()，但该函数只适用于工具别名。当前readNames()同时用于skills/mcpServers/通用权限字段，可能把合法技能名或 MCP 名称错误映射为工具名（如read→file_read）。🔧 建议修复
function readNames(value: unknown) { if (typeof value === 'string') { return value .split(/\s*,\s*|\s+/) .map((s) => s.trim()) .filter(Boolean) - .flatMap((s) => mapCompatToolName(s)) } if (!Array.isArray(value)) return [] return value .flatMap((item) => { if (typeof item === 'string') return item if (typeof item === 'object' && item != null) { const keys = Object.keys(item as Record<string, unknown>) return keys.length > 0 ? keys[0] : [] } return [] }) .map((s) => s.trim()) .filter(Boolean) - .flatMap((s) => mapCompatToolName(s)) }并在仅工具解析处显式映射（例如 Claude/OpenCode 的 tools 分支）。
Also applies to: 119-124, 283-287
🤖 Prompt for 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. In `@packages/extension-agent/src/sub-agent/parse.ts` around lines 374 - 397, The readNames function incorrectly applies mapCompatToolName to every name (affecting skills and mcpServers); change readNames to return raw normalized names without calling mapCompatToolName, and instead apply mapCompatToolName only at the tool-specific parsing locations (e.g., where Claude/OpenCode tools are handled). Concretely: remove or gate the .flatMap((s) => mapCompatToolName(s)) call inside readNames (or add a boolean parameter like applyToolMapping defaulting to false), update callers that expect tool alias mapping to call mapCompatToolName explicitly after readNames (or pass true), and ensure callers for skills and mcpServers use the unmapped result so skill/MCP names are not transformed.
🧹 Nitpick comments (3)
packages/extension-agent/src/computer/background.ts (1)
67-80: 💤 Low value
lines.pop()返回类型为string | undefined，可能导致类型推断问题。虽然
split(/\r?\n/)总是返回至少一个元素，所以运行时lines.pop()不会返回undefined，但 TypeScript 仍会将rest推断为string | undefined。如果启用严格空检查，{ pending: rest }可能导致类型错误。♻️ 建议添加非空断言以明确类型
const lines = (pending + data).split(/\r?\n/) - const rest = lines.pop() + const rest = lines.pop()!🤖 Prompt for 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. In `@packages/extension-agent/src/computer/background.ts` around lines 67 - 80, The variable `rest` is inferred as `string | undefined` because `lines.pop()` can return undefined; update the `const rest = lines.pop()` declaration in the block that iterates `lines` (using the same `lines`, `rest`, and `marker` identifiers) to assert non-null so the returned object `{ pending: rest }` matches the expected `string` type—e.g. replace the assignment with a non-null assertion (or a `?? ''` fallback) so `rest` is definitively a `string` before returning.packages/extension-agent/src/computer/backends/open_terminal.ts (1)
239-309: 💤 Low value
editFile中检测多重匹配的 indexOf 偏移量与 e2b.ts 不一致。这里使用
content.indexOf(oldString) + oldString.length作为第二次查找的起始偏移，而e2b.ts(line 305) 使用firstIdx + 1。两种写法都能正确检测第二次出现，但firstIdx + 1更简洁且与另一个后端实现保持一致。♻️ 建议保持一致性
if (replaceCount === 1) { + const firstIdx = content.indexOf(oldString) if ( content.indexOf( oldString, - content.indexOf(oldString) + oldString.length + firstIdx + 1 ) !== -1 ) {🤖 Prompt for 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. In `@packages/extension-agent/src/computer/backends/open_terminal.ts` around lines 239 - 309, The multi-match detection in editFile uses content.indexOf(oldString, content.indexOf(oldString) + oldString.length) which is inconsistent with the e2b backend; change it to capture the first match index (e.g., const firstIdx = content.indexOf(oldString)) and then check for a second match with content.indexOf(oldString, firstIdx + 1) so the logic is simpler and consistent with the e2b.ts implementation; update the editFile function accordingly (use firstIdx variable) while keeping the same error throw when a second match is found.packages/extension-agent/src/service/trigger.ts (1)
692-710: 💤 Low value建议使用
??=简化 Map 初始化。当前模式先检查再设置，可以用更简洁的写法。
♻️ 建议的简化写法
private _queueDeferred( key: string, pendingKey: string, item: DeferredWakeup ) { - if (!this._deferred.has(pendingKey)) { - this._deferred.set(pendingKey, new Map()) - } - this._deferred.get(pendingKey).set(key, item) + const map = this._deferred.get(pendingKey) ?? new Map<string, DeferredWakeup>() + map.set(key, item) + this._deferred.set(pendingKey, map) }🤖 Prompt for 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. In `@packages/extension-agent/src/service/trigger.ts` around lines 692 - 710, In _queueDeferred, replace the explicit check-and-set with a nullish-coalescing fallback when initializing the per-key Map: instead of the if (!this._deferred.has(pendingKey)) { this._deferred.set(pendingKey, new Map()) } pattern, use this._deferred.set(pendingKey, this._deferred.get(pendingKey) ?? new Map()) so the entry is created only if missing, then call this._deferred.get(pendingKey).set(key, item); keep the rest of _queueDeferred and _replayDeferred unchanged.
🤖 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/src/computer/backends/e2b.ts`:
- Around line 504-506: The getDesktopInfo method currently returns null while
its signature is Promise<DesktopInfo | undefined>, causing a type mismatch;
update the implementation of getDesktopInfo to return undefined instead of null
(or if null is intended, change the return type to include null), locating the
function named getDesktopInfo and the DesktopInfo type to ensure the return
value matches the declared Promise<DesktopInfo | undefined>.
In `@packages/extension-agent/src/config/defaults.ts`:
- Around line 47-49: Remove the banned helper function normalizeMode and inline
its logic at every call site: replace calls to normalizeMode(value) with the
conditional expression (value === 'allow' || value === 'deny' ? value : 'all');
update the callers createToolItemConfig and createSkillItemConfig to use that
inlined expression where they currently invoke normalizeMode, then delete the
normalizeMode function declaration and any imports/refs to it.
In `@packages/extension-agent/src/config/read.ts`:
- Around line 63-68: The current items construction replaces default
base.tool.items with cfg?.items, losing defaults; instead merge base.tool.items
with cfg?.items (shallow, so cfg overrides same keys) then feed the merged
object into Object.entries before mapping to createToolItemConfig; update the
items assignment that uses Object.fromEntries(... Object.entries(cfg?.items ??
{}) ...) to use the merged object (referencing base.tool.items, cfg?.items, and
createToolItemConfig) so missing or partial user configs preserve default tool
items.
In `@packages/extension-agent/src/sub-agent/parse.ts`:
- Around line 63-67: The detection logic treats 'maxTurns' as evidence for
Claude (the clause checking 'disallowedTools' || 'permissionMode' ||
'maxTurns'), but ChatLuna also supports 'maxTurns' in its branch (see the
chatluna branch around the frontmatter handling at lines ~157-160), causing
ChatLuna agents with maxTurns to be misclassified as Claude; fix by removing
'maxTurns' from the Claude-detection condition (or alternatively move the
ChatLuna-specific detection to run before the Claude check) so that only
properties unique to Claude (e.g., 'disallowedTools' or 'permissionMode')
trigger the Claude branch while 'maxTurns' is left to the ChatLuna branch
(update the condition that references frontmatter accordingly).
In `@packages/extension-agent/src/sub-agent/scan.ts`:
- Around line 73-77: The hint selection in scan.ts incorrectly only checks
combined.includes('/claude/agents') so paths like '.claude/agents' miss and fall
back to 'chatluna'; update the condition that assigns hint (the ternary using
combined and ScanTarget['hint']) to detect both variants — e.g., check
combined.includes('claude/agents') (or add an OR for
combined.includes('.claude/agents')) so that any path containing "claude/agents"
sets hint to 'claude' and preserves correct later defaulting of format.
In `@packages/extension-agent/src/utils/agentcli_sync.ts`:
- Around line 149-150: The helper isMissingFileError calls (err as
Error).message.toLowerCase() which can throw if err isn't an Error; change the
guard to safely extract a message: check for NodeJS.ErrnoException code 'ENOENT'
first, then derive msg using a safe expression (e.g., if err is an object with a
message use that, otherwise stringify err) and call toLowerCase() on the
resulting string so non-Error values (null, string, etc.) won't cause a
TypeError; update the code around isMissingFileError to use this safe message
extraction.
---
Outside diff comments:
In `@packages/extension-agent/src/sub-agent/parse.ts`:
- Around line 374-397: The readNames function incorrectly applies
mapCompatToolName to every name (affecting skills and mcpServers); change
readNames to return raw normalized names without calling mapCompatToolName, and
instead apply mapCompatToolName only at the tool-specific parsing locations
(e.g., where Claude/OpenCode tools are handled). Concretely: remove or gate the
.flatMap((s) => mapCompatToolName(s)) call inside readNames (or add a boolean
parameter like applyToolMapping defaulting to false), update callers that expect
tool alias mapping to call mapCompatToolName explicitly after readNames (or pass
true), and ensure callers for skills and mcpServers use the unmapped result so
skill/MCP names are not transformed.
---
Nitpick comments:
In `@packages/extension-agent/src/computer/backends/open_terminal.ts`:
- Around line 239-309: The multi-match detection in editFile uses
content.indexOf(oldString, content.indexOf(oldString) + oldString.length) which
is inconsistent with the e2b backend; change it to capture the first match index
(e.g., const firstIdx = content.indexOf(oldString)) and then check for a second
match with content.indexOf(oldString, firstIdx + 1) so the logic is simpler and
consistent with the e2b.ts implementation; update the editFile function
accordingly (use firstIdx variable) while keeping the same error throw when a
second match is found.
In `@packages/extension-agent/src/computer/background.ts`:
- Around line 67-80: The variable `rest` is inferred as `string | undefined`
because `lines.pop()` can return undefined; update the `const rest =
lines.pop()` declaration in the block that iterates `lines` (using the same
`lines`, `rest`, and `marker` identifiers) to assert non-null so the returned
object `{ pending: rest }` matches the expected `string` type—e.g. replace the
assignment with a non-null assertion (or a `?? ''` fallback) so `rest` is
definitively a `string` before returning.
In `@packages/extension-agent/src/service/trigger.ts`:
- Around line 692-710: In _queueDeferred, replace the explicit check-and-set
with a nullish-coalescing fallback when initializing the per-key Map: instead of
the if (!this._deferred.has(pendingKey)) { this._deferred.set(pendingKey, new
Map()) } pattern, use this._deferred.set(pendingKey,
this._deferred.get(pendingKey) ?? new Map()) so the entry is created only if
missing, then call this._deferred.get(pendingKey).set(key, item); keep the rest
of _queueDeferred and _replayDeferred unchanged.
🪄 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: fa20caf3-c93c-4deb-bd9a-a05095539374
📒 Files selected for processing (56)
packages/extension-agent/src/computer/backends/e2b.tspackages/extension-agent/src/computer/backends/local/index.tspackages/extension-agent/src/computer/backends/local/sandbox.tspackages/extension-agent/src/computer/backends/local/security.tspackages/extension-agent/src/computer/backends/local/shell.tspackages/extension-agent/src/computer/backends/local/store.tspackages/extension-agent/src/computer/backends/open_terminal.tspackages/extension-agent/src/computer/backends/types.tspackages/extension-agent/src/computer/background.tspackages/extension-agent/src/computer/materialize.tspackages/extension-agent/src/computer/proxy.tspackages/extension-agent/src/computer/session.tspackages/extension-agent/src/computer/tools/base.tspackages/extension-agent/src/computer/tools/bash.tspackages/extension-agent/src/computer/tools/file_edit.tspackages/extension-agent/src/computer/tools/file_read.tspackages/extension-agent/src/computer/tools/file_write.tspackages/extension-agent/src/computer/tools/glob.tspackages/extension-agent/src/computer/tools/grep.tspackages/extension-agent/src/computer/tools/publish_file.tspackages/extension-agent/src/config/defaults.tspackages/extension-agent/src/config/read.tspackages/extension-agent/src/service/computer.tspackages/extension-agent/src/service/index.tspackages/extension-agent/src/service/mcp.tspackages/extension-agent/src/service/permissions.tspackages/extension-agent/src/service/skills.tspackages/extension-agent/src/service/sub_agent.tspackages/extension-agent/src/service/trigger.tspackages/extension-agent/src/skills/builtin.tspackages/extension-agent/src/skills/catalog.tspackages/extension-agent/src/skills/import.tspackages/extension-agent/src/skills/manage.tspackages/extension-agent/src/skills/render.tspackages/extension-agent/src/skills/scan.tspackages/extension-agent/src/skills/slash.tspackages/extension-agent/src/skills/tool.tspackages/extension-agent/src/skills/watch.tspackages/extension-agent/src/sub-agent/builtin.tspackages/extension-agent/src/sub-agent/catalog.tspackages/extension-agent/src/sub-agent/manual.tspackages/extension-agent/src/sub-agent/markdown.tspackages/extension-agent/src/sub-agent/parse.tspackages/extension-agent/src/sub-agent/preset.tspackages/extension-agent/src/sub-agent/render.tspackages/extension-agent/src/sub-agent/run.tspackages/extension-agent/src/sub-agent/runtime.tspackages/extension-agent/src/sub-agent/scan.tspackages/extension-agent/src/sub-agent/session.tspackages/extension-agent/src/sub-agent/tool.tspackages/extension-agent/src/utils/agentcli_sync.tspackages/extension-agent/src/utils/fs.tspackages/extension-agent/src/utils/path.tspackages/extension-agent/src/utils/remote_path.tspackages/extension-agent/src/utils/runtime_sync.tspackages/extension-agent/src/utils/shadow.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/extension-agent/src/trigger/executor.ts`:
- Around line 274-279: 检查分支在判断 target 是否为对象并使用 'bot' in target 时缺乏对 null 的保护；在
executor.ts 中定位使用 target、session 和类型 Session 的代码块并在结构性检查前添加 target !==
null（或等效的非空检查），例如把 typeof target === 'object' 改为先确认 target !== null 然后再检查 'bot'
in target，以避免在 target 为 null 时抛出 TypeError。
🪄 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: 23a5dcc4-5b64-43ee-9555-139542554994
📒 Files selected for processing (28)
packages/extension-agent/src/commands/agent.tspackages/extension-agent/src/commands/mcp.tspackages/extension-agent/src/computer/backends/e2b.tspackages/extension-agent/src/computer/backends/open_terminal.tspackages/extension-agent/src/computer/background.tspackages/extension-agent/src/config/defaults.tspackages/extension-agent/src/config/read.tspackages/extension-agent/src/index.tspackages/extension-agent/src/mcp/content.tspackages/extension-agent/src/mcp/storage.tspackages/extension-agent/src/mcp/tool_call.tspackages/extension-agent/src/mcp/transport.tspackages/extension-agent/src/service/trigger.tspackages/extension-agent/src/sub-agent/parse.tspackages/extension-agent/src/sub-agent/run.tspackages/extension-agent/src/sub-agent/scan.tspackages/extension-agent/src/trigger/dsl.tspackages/extension-agent/src/trigger/executor.tspackages/extension-agent/src/trigger/listener.tspackages/extension-agent/src/trigger/provider_registry.tspackages/extension-agent/src/trigger/providers/activity.tspackages/extension-agent/src/trigger/providers/cron.tspackages/extension-agent/src/trigger/render.tspackages/extension-agent/src/trigger/scheduler.tspackages/extension-agent/src/trigger/task_registry.tspackages/extension-agent/src/trigger/tool.tspackages/extension-agent/src/utils/agentcli_sync.tspackages/extension-agent/src/webui/index.ts
✅ Files skipped from review due to trivial changes (4)
- packages/extension-agent/src/webui/index.ts
- packages/extension-agent/src/trigger/providers/cron.ts
- packages/extension-agent/src/trigger/listener.ts
- packages/extension-agent/src/mcp/storage.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/extension-agent/src/utils/agentcli_sync.ts
- packages/extension-agent/src/config/read.ts
- packages/extension-agent/src/sub-agent/run.ts
- packages/extension-agent/src/sub-agent/parse.ts
- packages/extension-agent/src/computer/backends/e2b.ts
- packages/extension-agent/src/sub-agent/scan.ts
- packages/extension-agent/src/computer/background.ts
- packages/extension-agent/src/service/trigger.ts
- packages/extension-agent/src/computer/backends/open_terminal.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 simplifies the ChatLuna agent extension implementation across computer backends, tools, skills, sub-agents, and service wiring.
New Features
None
Bug fixes
Other Changes
yarn lint-fixcompleted with no errors. Existing max-len warnings remain inread_chat_message.tsandextension-agent/src/sub-agent/builtin.ts.