fix(extension-agent): restore keep-alive sandbox reconnects and skill bin checks#791
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the reliability of E2B sandbox interactions by handling disconnections and timeouts more gracefully. It also refines how the system verifies the presence of necessary binaries for skills, making these checks more accurate across various execution environments. Additionally, the user interface for managing skills has been tidied up for a better user experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (12)
Walkthrough在多处引入并传递上下文（ctx），新增远程技能/子代理扫描与移除、hasBin 检测与 agentcli 工具，增强 E2B 沙箱恢复与错误分类；导出若干本地 shell 发现函数，调整技能/子代理删除流与 UI 布局，引入并传播 remote 标记。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client UI
participant AgentService as Agent Service
participant Computer as Computer Service
participant Backend as Backend (E2B / Local)
UI->>AgentService: 请求扫描 / 删除 / 导出（含 ctx）
AgentService->>Computer: scanRemoteSkills(ctx) / hasBin(name) / removeRemoteSkill(path)
alt 后端为远程（E2B）
Computer->>Backend: 建立或复用会话（_connecting）并执行 probe/rm/run
Backend-->>Computer: 返回 exit code / 输出 / 错误（可能为 missing-sandbox）
else 本地
Computer->>Backend: 本地探测（which / findPowerShell / findGitBash）
Backend-->>Computer: 返回路径或存在性
end
Computer-->>AgentService: 返回结果或错误（已包装消息）
AgentService-->>UI: 更新 catalog / 触发 refresh / 显示成功或错误提示
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)
📝 Coding Plan
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 significantly enhances the agent's capabilities by improving E2B sandbox management and introducing robust skill binary checks. The E2B sandbox connection logic now gracefully handles cases where a kept-alive sandbox might be unavailable, ensuring a new instance is created. The introduction of a centralized run method in E2BComputerSession streamlines command execution and ensures sandbox timeouts are consistently reset. Furthermore, a comprehensive mechanism for checking required skill binaries has been implemented, supporting local environments (including Windows-specific shells) and remote sandboxes. The UI changes in skills-page.vue also improve the layout and accessibility of skill actions. Overall, these changes contribute to a more reliable and functional agent.
![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: 3
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/skills/import.ts (1)
122-129:⚠️ Potential issue | 🔴 Critical
ctx不在previewMaterializedSource()的作用域内，当前代码无法通过 TypeScript 编译。Line 129 调用
scanSkillRoot(root, ctx)但ctx没有定义。需要为previewMaterializedSource()添加ctx: Context参数，并在两处调用点传入ctx。修复
async function previewMaterializedSource( + ctx: Context, root: string, source: SkillImportInput['type'], target: string, diagnostics: string[] ): Promise<SkillImportPreviewResult> { const entries = await collectPreviewEntries(root) const scanned = await scanSkillRoot(root, ctx)- return await previewMaterializedSource( - source.root, + return await previewMaterializedSource( + ctx, + source.root, input.type, input.name, source.diagnostics )- const preview = await previewMaterializedSource( - source.root, + const preview = await previewMaterializedSource( + ctx, + source.root, input.type, input.type === 'zip' ? stripExt(input.name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/skills/import.ts` around lines 122 - 129, previewMaterializedSource currently calls scanSkillRoot(root, ctx) but ctx is undefined; add a new parameter ctx: Context to the previewMaterializedSource function signature and update both call sites to pass the Context through. Locate previewMaterializedSource and change its signature to accept ctx, then update the two places that invoke previewMaterializedSource to forward the existing Context object so scanSkillRoot(root, ctx) uses a valid ctx.
🤖 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/computer/backends/e2b.ts`:
- Around line 78-93: The current connect/create fallback in ensureSandbox() and
the try/catch around E2BSandbox.connect(...) (and the swallowed exception from
this._sandbox.internal.isRunning()) indiscriminately treats all errors as
"sandbox missing" and recreates a sandbox; change it so you only fall back to
E2BSandbox.create(...) when the error is a concrete "not found"/"terminated"
sandbox error (e.g., specific error.code or E2BSandboxError type/message), and
for other exceptions rethrow (or surface them to be retried by caller) so
transient/network/API errors do not trigger create() and leak resources; update
the catch blocks around E2BSandbox.connect, this._sandbox.internal.isRunning(),
and connect() to inspect the error and only call create() when it matches the
terminated/not-exist condition, otherwise propagate the error.
- Around line 105-112: connect() initializes the sandbox timeout from
this.cfg.timeoutMs but run()'s finally currently overwrites it with the
hardcoded SANDBOX_COMMAND_TIMEOUT, which permanently erases user-configured
timeouts; fix run() so it does not reset the sandbox to the hardcoded
value—either save the sandbox's previous timeout before executing the command
and restore that previous value in the finally block, or restore
this.cfg.timeoutMs instead of SANDBOX_COMMAND_TIMEOUT—and remove the
SANDBOX_COMMAND_TIMEOUT constant definition entirely; update references in run()
and any callers to rely on the restored/ cfg timeout behavior.
In `@packages/extension-agent/src/service/computer.ts`:
- Around line 420-461: Normalize Windows shell aliases (map 'pwsh'↔'powershell',
'bash'↔'sh', 'cmd'↔'cmd.exe') before performing the permission checks so
blockedCommands/allowedCommands comparisons use the canonical name; move or
repeat the alias-mapping logic to run prior to the checks that reference
this.config.computer.local.blockedCommands and allowedCommands (and still keep
the platform-specific detection using findGitBash() and findPowerShell() after
mapping). Update comparisons that call name.toLowerCase() to use the
normalized/canonical command string, and ensure the process.platform === 'win32'
handling (including calls to findGitBash() and findPowerShell()) uses the same
normalization to avoid mismatches.
---
Outside diff comments:
In `@packages/extension-agent/src/skills/import.ts`:
- Around line 122-129: previewMaterializedSource currently calls
scanSkillRoot(root, ctx) but ctx is undefined; add a new parameter ctx: Context
to the previewMaterializedSource function signature and update both call sites
to pass the Context through. Locate previewMaterializedSource and change its
signature to accept ctx, then update the two places that invoke
previewMaterializedSource to forward the existing Context object so
scanSkillRoot(root, ctx) uses a valid ctx.
🪄 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: ec53f12b-4599-4834-b708-1970ee749971
📒 Files selected for processing (6)
packages/extension-agent/client/components/skills/skills-page.vuepackages/extension-agent/src/computer/backends/e2b.tspackages/extension-agent/src/computer/backends/local/shell.tspackages/extension-agent/src/service/computer.tspackages/extension-agent/src/skills/import.tspackages/extension-agent/src/skills/scan.ts
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 (1)
packages/extension-agent/src/computer/backends/e2b.ts (1)
108-141:⚠️ Potential issue | 🟠 Major不要在初始化完成前写回
_sandbox状态。Line 108-109 先把
_sandbox/_sandboxId挂到实例上，但后面的pwd和cwd探测还会继续await。如果这些步骤里任一步失败，当前对象会留下一个“活着但未完成初始化”的沙箱；后续ensureSandbox()只看isRunning()，会直接复用它，_home/_root/_cwd可能还是旧值。把实例字段赋值移到初始化全部成功之后，或在失败时显式回滚这些字段。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/computer/backends/e2b.ts` around lines 108 - 141, The instance assigns this._sandbox and this._sandboxId before performing async initialization steps (calls to sandbox.setTimeout, this.run('pwd'), resolvePath and directory probe), which can leave a partially-initialized sandbox on failure; change the flow in the initialization routine so that you only set this._sandbox and this._sandboxId after all async probes succeed (and after setting this._home, this._root, this._cwd and this._connected), or if you prefer to keep early assignment wrap the async block in try/catch and explicitly roll back/clear this._sandbox, this._sandboxId, this._home, this._root, this._cwd and this._connected in the catch before rethrowing; update usages of run, resolvePath and setTimeout accordingly and ensure ensureSandbox()/isRunning() will not see a live sandbox until initialization completes.
🧹 Nitpick comments (1)
packages/extension-agent/src/cli/dispatch.ts (1)
1817-1840: 同一个远端文件下的本地根检查可以并行。现在每个
localRoot都串行readFile，兼容根越多，agentcli sync预览越慢。这里适合至少对单个远端文件对应的本地目标用Promise.all。As per coding guidelines, 'ALWAYS USE PARALLEL TOOLS WHEN APPLICABLE'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/cli/dispatch.ts` around lines 1817 - 1840, The loop that checks each localRoot for a remote file is doing serial readFile calls causing slowness; in the block that iterates localRoots (inside the loop over listRemoteFiles and after reading content via session.readFile), run readFile for all targetPath candidates in parallel using Promise.all, then iterate the results to decide equality, increment unchanged, and push to the files array with the same properties (kind, path, sourcePath, targetPath, content, mode) but derived from the parallel read results; preserve behavior where current == null => mode 'create' and equality check uses strict === against content.
🤖 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/cli/dispatch.ts`:
- Line 1982: The help string "Skill sync fans out to ChatLuna and compatibility
directories such as .agents/skills, .openclaw/skills, .codex/skills,
.claude/skills, and OpenCode skill roots." exceeds the repo max line width;
split it into multiple concatenated string literals or a multi-line template
literal so no single source line goes over the limit (e.g., break after
"ChatLuna and", or after listing a few directories) and ensure the resulting
text is identical when joined; update the long string occurrence in dispatch.ts
(the help/usage text where that sentence appears) to the multi-line form.
- Around line 1823-1825: The current read of targetPath using
readFile(...).catch(() => undefined) hides all IO errors; update the read to
catch errors and only treat ENOENT as "file not found" (return undefined), but
rethrow any other errors (e.g., EACCES, EISDIR) so they are not mistaken for a
create case—locate the readFile usage that assigns current and replace the
blanket catch with an error handler that checks err.code === 'ENOENT' and
otherwise throws the error, ensuring downstream apply/writeFile logic sees real
I/O failures.
In `@packages/extension-agent/src/computer/backends/e2b.ts`:
- Around line 496-515: ensureSandbox currently calls this.connect() from
multiple paths causing concurrent reconnects and sandbox leakage; introduce a
shared pending connection promise (e.g., this._connectPromise) that
ensureSandbox checks and awaits instead of calling connect() directly, have
connect() set this._connectPromise at start and clear it on success or failure,
ensure callers await the same promise so only one sandbox instance is created,
and preserve existing behavior for setting this._connected and this._sandbox
while clearing the pending promise on errors; update references in ensureSandbox
and connect to use this shared promise.
- Around line 518-535: 在 run 方法中目前 finally 块直接 throw err（来自
current.setTimeout），这会覆盖或替换 current.commands.run 的返回值或原始异常；修改 run 以保存 run
的结果或捕获到的原始异常，然后在调用 current.setTimeout
后再决定是否抛出清理错误（例如：记录/抑制非关键清理错误或在保留原始异常时优先抛出原始异常）；具体定位
run、ensureSandbox、current.setTimeout 和 isMissingSandboxError，改为先保存 run 的
outcome（返回值或捕获的 error），在 finally 中执行 setTimeout 并仅在没有原始错误且 setTimeout 失败且不是
isMissingSandboxError 时再抛出清理错误，或完全静默/记录清理错误以避免覆盖命令结果。
---
Outside diff comments:
In `@packages/extension-agent/src/computer/backends/e2b.ts`:
- Around line 108-141: The instance assigns this._sandbox and this._sandboxId
before performing async initialization steps (calls to sandbox.setTimeout,
this.run('pwd'), resolvePath and directory probe), which can leave a
partially-initialized sandbox on failure; change the flow in the initialization
routine so that you only set this._sandbox and this._sandboxId after all async
probes succeed (and after setting this._home, this._root, this._cwd and
this._connected), or if you prefer to keep early assignment wrap the async block
in try/catch and explicitly roll back/clear this._sandbox, this._sandboxId,
this._home, this._root, this._cwd and this._connected in the catch before
rethrowing; update usages of run, resolvePath and setTimeout accordingly and
ensure ensureSandbox()/isRunning() will not see a live sandbox until
initialization completes.
---
Nitpick comments:
In `@packages/extension-agent/src/cli/dispatch.ts`:
- Around line 1817-1840: The loop that checks each localRoot for a remote file
is doing serial readFile calls causing slowness; in the block that iterates
localRoots (inside the loop over listRemoteFiles and after reading content via
session.readFile), run readFile for all targetPath candidates in parallel using
Promise.all, then iterate the results to decide equality, increment unchanged,
and push to the files array with the same properties (kind, path, sourcePath,
targetPath, content, mode) but derived from the parallel read results; preserve
behavior where current == null => mode 'create' and equality check uses strict
=== against content.
🪄 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: 5ee96a5c-3ec4-4176-aae7-b2008b6e586d
⛔ Files ignored due to path filters (1)
packages/extension-agent/package.jsonis excluded by!**/*.json
📒 Files selected for processing (8)
packages/extension-agent/src/cli/dispatch.tspackages/extension-agent/src/computer/backends/e2b.tspackages/extension-agent/src/config/defaults.tspackages/extension-agent/src/config/path.tspackages/extension-agent/src/index.tspackages/extension-agent/src/skills/import.tspackages/extension-agent/src/skills/scan.tspackages/extension-agent/src/types/skills.ts
✅ Files skipped from review due to trivial changes (1)
- packages/extension-agent/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-agent/src/skills/import.ts
- packages/extension-agent/src/skills/scan.ts
Register a dedicated agentcli tool so admin sessions stop routing through bash, and surface remote skill and sub-agent scanning plus removal in the service and UI.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
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/client/components/sub-agent/sub-agent-page.vue (1)
469-494:⚠️ Potential issue | 🟠 Major删除请求缺少进行中保护。
这里在确认后直接发起
removeSubAgent，但整个删除和刷新期间都没有把页面置为 busy。现在目录卡片上也有删除入口，用户可以在首个请求未完成前再次点删除或切换状态，结果会依赖后端删除是否幂等。建议确认通过后立刻锁住 UI（全局busy或按 item 的 pending 状态），并在finally中恢复。💡 一个最小改法
async function removeAgent(item: SubAgentInfo) { const selected = selectedId.value === item.id try { await ElMessageBox.confirm( `删除"${item.name}"后需要手动重新导入或重新创建，确定继续吗？`, '删除 Sub Agent', { confirmButtonText: '删除', cancelButtonText: '取消', type: 'warning' } ) + busy.value = true await send('chatluna-agent/removeSubAgent', item.id) if (selected) { currentView.value = 'list' } await loadExtraData() ElMessage.success('已删除该 sub-agent。') } catch (error) { if (error !== 'cancel' && error !== 'close') { ElMessage.error('删除失败，请稍后重试。') } + } finally { + busy.value = false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/client/components/sub-agent/sub-agent-page.vue` around lines 469 - 494, The removeAgent function lacks UI locking during the confirm/remove/refresh flow, so lock the UI immediately after confirmation (either set a global busy flag or mark the specific SubAgentInfo item with a pending/loading state) before calling send('chatluna-agent/removeSubAgent', item.id), ensure subsequent UI changes (currentView switching and loadExtraData) occur while locked, and always clear that busy/pending flag in a finally block to restore interactivity; use the existing symbols removeAgent, selectedId, currentView, loadExtraData and the item (SubAgentInfo) to implement the per-item pending state if preferred.
🧹 Nitpick comments (1)
packages/extension-agent/src/service/computer.ts (1)
835-836: 行长度超过限制。Line 836 长度为 170 字符，超过配置的最大 160 字符。建议拆分命令字符串。
建议修改
+ const cmd = [ + `if [ -d ${quoteShellPath(target)} ]; then rm -rf ${quoteShellPath(target)};`, + `elif [ -e ${quoteShellPath(target)} ]; then rm -f ${quoteShellPath(target)}; fi` + ].join(' ') + const result = await session.execute( - `if [ -d ${quoteShellPath(target)} ]; then rm -rf ${quoteShellPath(target)}; elif [ -e ${quoteShellPath(target)} ]; then rm -f ${quoteShellPath(target)}; fi`, + cmd, { timeout: 15000 } )🤖 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 835 - 836, The shell command string passed to session.execute (the template literal using quoteShellPath(target)) exceeds the max line length; split the long command into smaller concatenated parts or build it as a multi-line template literal so the line is under 160 chars. Locate the session.execute call around the code using quoteShellPath(target) and break the command into pieces (e.g., store the conditional rm command in a const cmdParts or use string concatenation/template lines) and then pass the assembled cmd to session.execute, preserving the same logic (check directory -> rm -rf, else if exists -> rm -f).
🤖 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/cli/render.ts`:
- Line 85: The long help string "'Command chains support `&`, `&&`, `|`, `|&`,
`||`, and `;` inside the `command` string. Pipe operators only separate agentcli
calls; they do not stream stdin.'" exceeds the max line width; locate that exact
string in render.ts and split it into multiple shorter string literals
concatenated (or use an array of strings joined) so no source line is longer
than the configured max (80 chars/prettier); keep the same wording and
punctuation, ensuring the final runtime string is identical.
In `@packages/extension-agent/src/computer/backends/e2b.ts`:
- Around line 618-626: The finally block currently throws errors from
current.setTimeout (via the catch that rethrows when
!isMissingSandboxError(err)), which can overwrite an earlier exception from
current.commands.run(); change the logic to preserve and rethrow the original
command error: capture the original exception (e.g., let originalErr) before
entering finally (or detect if one exists), then in the finally catch for
current.setTimeout, do not throw the new setTimeout error; instead, if
originalErr exists rethrow originalErr, otherwise rethrow the setTimeout error;
use the symbols current.setTimeout, current.commands.run, and
isMissingSandboxError to locate and implement this behavior.
In `@packages/extension-agent/src/computer/tools/bash.ts`:
- Around line 104-108: 当前拦截逻辑只在 parseAgentCliCommand(input.command ?? '')
成功时阻止执行，导致带有 agentcli 前缀但解析失败的命令仍会回落到 bash；请把条件改成只要检测到命令以 agentcli 开头即拒绝（例如在检查
action === 'run' 时用 input.command?.trim().match(/^agentcli\b/i) 或
startsWith('agentcli') 代替 parseAgentCliCommand 的成功判定），保留对 this.formatResult(...)
的返回行为来提示使用专用工具；参考标识符：parseAgentCliCommand、input.command、action、this.formatResult。
In `@packages/extension-agent/src/service/computer.ts`:
- Around line 476-485: The current check in session.execute double-escapes the
binary name by calling quoteShell(name) inside the inner template and again on
the outer sh -lc, causing command -v to fail; update the call that builds the
shell command so you only escape the full command once (remove the inner
quoteShell around name and use quoteShell on the whole string passed to sh -lc),
i.e. construct `command -v ${name} >/dev/null 2>&1` without quoting name
separately and then call quoteShell(...) around that full command before passing
it to session.execute; keep using the same session.execute, timeout, and catch
behavior.
In `@packages/extension-agent/src/service/skills.ts`:
- Around line 111-115: The current call assumes this.ctx.chatluna_agent.computer
always exists and may invoke scanRemoteSkills on undefined; update the
invocation to use full optional chaining on both computer and method (e.g.,
this.ctx.chatluna_agent?.computer?.scanRemoteSkills?.()) so the call is skipped
if either is undefined, and still await the result with a fallback to an empty
array and proper error handling (keep or replace the current .catch(() => [])
with an explicit try/catch or a .catch that logs/errors as needed) to ensure no
unhandled exceptions from missing properties or promise rejections; reference
this.ctx.chatluna_agent, computer, and scanRemoteSkills to locate the code.
In `@packages/extension-agent/src/service/sub_agent.ts`:
- Around line 454-458: The current catch(() => []) on
this.ctx.chatluna_agent.computer.scanRemoteSubAgents() silently turns
remote-scan failures into an empty list causing refreshCatalog() to rebuild
_catalog incorrectly; remove the swallowing catch so the error can propagate (or
at minimum catch, log with context and rethrow) and ensure refreshCatalog() is
only called with a successful result from scanRemoteSubAgents(); reference
scanRemoteSubAgents, this.ctx.chatluna_agent, refreshCatalog, and _catalog when
making the change.
In `@packages/extension-agent/src/skills/catalog.ts`:
- Around line 56-58: The current check in the catalog loop that skips IDs
matching /^[a-f0-9]{16}$/i is wrong because createSkillId(file) produces
hash-style IDs for both local and remote skills; replace this heuristic with an
explicit remote-metadata check: remove the regex-based continue in the loop in
packages/extension-agent/src/skills/catalog.ts and instead detect remote skills
using the scanned skill entry’s metadata (e.g. a property such as
entry.metadata.remote, entry.source === 'remote', or whatever flag the scanner
in packages/extension-agent/src/skills/scan.ts sets) so only true remote entries
are skipped and missing local markdown skills will be emitted as state:
'missing'.
In `@packages/extension-agent/src/skills/render.ts`:
- Around line 93-95: 当前代码在构建 `resources` 时用 `.catch(() => [] as string[])` 将
`listSkillResources(skill.dir)` 的 I/O 错误吞掉并当作“无资源”处理；请移除该降级处理：如果
`options.resources` 已显式提供则直接使用它，否则直接 `await listSkillResources(skill.dir)`
不捕获异常，让真实错误冒泡；定位并修改包含 `resources = options.resources ?? (await
listSkillResources(skill.dir).catch(() => [] as string[]))` 的表达式（注意
`listSkillResources`、`options.resources`、`skill.dir` 标识符）以实现上述行为。
In `@packages/extension-agent/src/skills/scan.ts`:
- Around line 200-220: listRemoteSkillResources fails on BSD because the shell
command uses GNU-only "find -printf '%P\n'"; replace the remote command passed
to session.execute with a POSIX-compatible pipeline: use "find ... -type f !
-name SKILL.md -print" (with the same quoteShellPath(dir) usage) and then strip
the leading directory prefix using a portable tool like sed or awk in the same
command so the output returns relative paths; keep the existing stderr check,
timeout, and the function name listRemoteSkillResources unchanged.
In `@packages/extension-agent/src/sub-agent/scan.ts`:
- Around line 340-386: There are duplicated path utilities with divergent
behavior (missing `.openclaw/`) and small helpers extracted unnecessarily;
update so implementations are consistent and follow the coding guideline: keep a
single non-"resolve" top-level function (rename resolveRemoteDir to e.g.
computeRemoteDir) with identical logic to the counterpart (ensure the
`.openclaw/` check is included), inline the tiny helpers trimRemotePrefix,
normalizeRemoteBase, and normalizeRemoteKey into their call sites inside
computeRemoteDir and isRemotePathInside, and remove the separate extracted
functions or ensure all usages use the unified implementation so both modules
behave the same.
In `@packages/extension-agent/src/utils/shadow.ts`:
- Around line 27-35: The current selection picks winner = list[0] after sorting
by remote and priority, which can choose entries that are disabled/invalid or
available === false and thus incorrectly shadow runnable items; before assigning
winner (in the logic around list.sort and winner), filter the list to exclude
entries with disabled === true, invalid === true, or available === false (or
otherwise treat them as non-candidates), then sort the filtered candidates by
the existing remote/priority comparator and pick the first viable candidate;
ensure the same filtering logic applies where shadowing decisions are made so
that shadowedBy/visible calculations (used downstream) only consider
runnable/available entries.
In `@packages/extension-agent/src/utils/shell.ts`:
- Around line 12-13: The branch handling when value === '~' returns an unquoted
$HOME which is not shell-safe; change that branch so it returns the $HOME token
wrapped in double quotes (i.e. a string that yields "$HOME" to the shell)
matching the quoting used by the '~/...' branch so paths with spaces/special
characters are protected; update the if (value === '~') return to produce the
double-quoted form and keep the rest of the function behavior unchanged.
---
Outside diff comments:
In `@packages/extension-agent/client/components/sub-agent/sub-agent-page.vue`:
- Around line 469-494: The removeAgent function lacks UI locking during the
confirm/remove/refresh flow, so lock the UI immediately after confirmation
(either set a global busy flag or mark the specific SubAgentInfo item with a
pending/loading state) before calling send('chatluna-agent/removeSubAgent',
item.id), ensure subsequent UI changes (currentView switching and loadExtraData)
occur while locked, and always clear that busy/pending flag in a finally block
to restore interactivity; use the existing symbols removeAgent, selectedId,
currentView, loadExtraData and the item (SubAgentInfo) to implement the per-item
pending state if preferred.
---
Nitpick comments:
In `@packages/extension-agent/src/service/computer.ts`:
- Around line 835-836: The shell command string passed to session.execute (the
template literal using quoteShellPath(target)) exceeds the max line length;
split the long command into smaller concatenated parts or build it as a
multi-line template literal so the line is under 160 chars. Locate the
session.execute call around the code using quoteShellPath(target) and break the
command into pieces (e.g., store the conditional rm command in a const cmdParts
or use string concatenation/template lines) and then pass the assembled cmd to
session.execute, preserving the same logic (check directory -> rm -rf, else if
exists -> rm -f).
🪄 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: 8db4ef0d-6da2-4117-b545-761662a05c21
⛔ Files ignored due to path filters (1)
packages/extension-agent/package.jsonis excluded by!**/*.json
📒 Files selected for processing (30)
packages/extension-agent/client/components/computer/computer-page.vuepackages/extension-agent/client/components/layout/agent-shell.vuepackages/extension-agent/client/components/mcp/mcp-page.vuepackages/extension-agent/client/components/skills/skills-page.vuepackages/extension-agent/client/components/sub-agent/sub-agent-catalog.vuepackages/extension-agent/client/components/sub-agent/sub-agent-detail.vuepackages/extension-agent/client/components/sub-agent/sub-agent-page.vuepackages/extension-agent/client/components/tool/tool-page.vuepackages/extension-agent/resources/skills/agent-config-admin/SKILL.mdpackages/extension-agent/src/cli/render.tspackages/extension-agent/src/cli/service.tspackages/extension-agent/src/cli/tool.tspackages/extension-agent/src/cli/types.tspackages/extension-agent/src/computer/backends/e2b.tspackages/extension-agent/src/computer/materialize.tspackages/extension-agent/src/computer/tools/bash.tspackages/extension-agent/src/config/defaults.tspackages/extension-agent/src/service/computer.tspackages/extension-agent/src/service/index.tspackages/extension-agent/src/service/skills.tspackages/extension-agent/src/service/sub_agent.tspackages/extension-agent/src/skills/catalog.tspackages/extension-agent/src/skills/render.tspackages/extension-agent/src/skills/scan.tspackages/extension-agent/src/sub-agent/catalog.tspackages/extension-agent/src/sub-agent/scan.tspackages/extension-agent/src/types/skills.tspackages/extension-agent/src/types/sub_agent.tspackages/extension-agent/src/utils/shadow.tspackages/extension-agent/src/utils/shell.ts
✅ Files skipped from review due to trivial changes (5)
- packages/extension-agent/client/components/sub-agent/sub-agent-detail.vue
- packages/extension-agent/src/service/index.ts
- packages/extension-agent/src/types/sub_agent.ts
- packages/extension-agent/resources/skills/agent-config-admin/SKILL.md
- packages/extension-agent/client/components/layout/agent-shell.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-agent/src/config/defaults.ts
- packages/extension-agent/src/types/skills.ts
Prevent concurrent keep-alive reconnects from creating duplicate sandboxes or duplicate session entries for the same conversation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/extension-agent/src/computer/backends/e2b.ts (1)
640-658:⚠️ Potential issue | 🟠 Major
finally块中的throw会覆盖原始结果或异常。当
current.commands.run()成功返回但setTimeout()抛出非 missing-sandbox 错误时，调用方会收到 setTimeout 的异常而非命令结果。更糟糕的是，如果命令执行本身抛出异常，也会被 setTimeout 的错误覆盖，导致真正的错误信息丢失。🐛 建议修复：保留原始异常优先级
private async run( command: string, options?: CommandStartOpts, sandbox?: SandboxWrapper ) { const current = sandbox ?? (await this.ensureSandbox()) + let commandError: unknown try { return await current.commands.run(command, options) + } catch (err) { + commandError = err + throw err } finally { try { await current.setTimeout(this.cfg.timeoutMs) } catch (err) { - if (!isMissingSandboxError(err)) { + if (!isMissingSandboxError(err) && !commandError) { throw err } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extension-agent/src/computer/backends/e2b.ts` around lines 640 - 658, In run, the finally block's throw from current.setTimeout can overwrite the original result or exception from current.commands.run; change run to capture the run result or error (e.g., store result in a local variable and store any run exception), then call current.setTimeout and only propagate the setTimeout error when there was no prior run error (or attach it as secondary/cause) and when it isn't a missing-sandbox error via isMissingSandboxError; ensure commands.run result is returned when successful and original run exceptions are rethrown if present, while still handling setTimeout errors as non-fatal when appropriate.
🤖 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/computer/backends/e2b.ts`:
- Around line 703-722: The isMissingSandboxError function contains speculative
message-based checks; update function isMissingSandboxError to remove the
fallback message matching (the checks against err.message including 'not found',
'not running anymore', 'terminated') and instead only return true for actual
NotFoundError types — keep the initial instanceof NotFoundError check, ensure
non-Error values still return false, and then simply return err.name ===
'NotFoundError' for Error instances; adjust control flow accordingly so no
message string inspections remain.
---
Duplicate comments:
In `@packages/extension-agent/src/computer/backends/e2b.ts`:
- Around line 640-658: In run, the finally block's throw from current.setTimeout
can overwrite the original result or exception from current.commands.run; change
run to capture the run result or error (e.g., store result in a local variable
and store any run exception), then call current.setTimeout and only propagate
the setTimeout error when there was no prior run error (or attach it as
secondary/cause) and when it isn't a missing-sandbox error via
isMissingSandboxError; ensure commands.run result is returned when successful
and original run exceptions are rethrown if present, while still handling
setTimeout errors as non-fatal when appropriate.
🪄 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: f0b3716d-e14e-4df8-87d3-d52896d1d1ce
⛔ Files ignored due to path filters (1)
packages/extension-agent/package.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
packages/extension-agent/src/computer/backends/e2b.tspackages/extension-agent/src/computer/session.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.
Summary