fix(sandbox): ensure skills and attachments are ready - #2587
Conversation
📝 WalkthroughWalkthroughThe pull request adds versioned built-in skill updates, secure attachment handling, chat-to-sandbox attachment synchronization, sandbox skill deployment, and executor failure propagation for required skills. ChangesSandbox skill and attachment flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: d4a6d9e9a9
ℹ️ 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".
| # The attachment block exposes the browser download URL. Translate | ||
| # that exact Wegent route to the executor route because sandbox tools | ||
| # authenticate with a task token rather than a browser login token. | ||
| download_url = _build_download_url(attachment_url, api_base_url) |
There was a problem hiding this comment.
The tool argument is model-controlled, and this translation sends any numeric attachment ID to executor-download with the task token. That endpoint authenticates task tokens only to a user and filters the attachment by user_id, without checking the token's task_id; consequently, a prompt can enumerate IDs and download attachments belonging to the same user but unrelated tasks. Validate the requested attachment against the active task, or make the executor endpoint enforce the task claim before exposing this route through the tool.
Useful? React with 👍 / 👎.
| return [ | ||
| await _sync_one_attachment( | ||
| client=client, |
There was a problem hiding this comment.
Each attachment download is awaited inside the list comprehension, so requests run strictly serially before _process_chat can produce any response. Since every request has a 180-second timeout and API requests may contain many attachments, several slow or unavailable downloads can block a chat for multiples of that timeout (up to hours at the supported attachment limits). Use bounded concurrency or an overall synchronization deadline so one stalled download does not serialize the entire batch.
Useful? React with 👍 / 👎.
| filename = _attachment_filename(attachment) | ||
| path = str(attachment.get("local_path") or "") | ||
| download_url = build_attachment_download_url(attachment_id) | ||
| lines.append( | ||
| f"- {filename} (ID: {attachment_id}). Use download_attachment with " |
There was a problem hiding this comment.
When synchronization fails, the attachment's original filename is interpolated directly into the model prompt even though filenames may contain control characters or path components. This bypasses the new sanitize_attachment_filename protection used by the normal attachment header, so a crafted filename containing a newline can inject arbitrary prompt text specifically on the failure path. Sanitize the filename before constructing this warning.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
chat_shell/chat_shell/services/sandbox_attachment_sync.py (2)
250-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog attachment synchronization failures.
Both failure branches record the error only in the attachment dictionary. Nothing reaches the logs, so an operator cannot diagnose a failed download from service logs. Add a warning log with
task_id,attachment_id, and the error.The project guideline states: "Diagnose problems using logs, actual code, and other concrete evidence first; when evidence is insufficient, add focused diagnostic logging before changing behavior and do not guess." As per coding guidelines.
♻️ Proposed logging
except httpx.HTTPStatusError as exc: + logger.warning( + "[sandbox_attachment_sync] Download failed: task_id=%s, attachment_id=%s, " + "status=%s", + request.task_id, + attachment_id, + exc.response.status_code, + ) updated.update( { "status": "failed", "error": f"Attachment download returned HTTP {exc.response.status_code}", } ) except Exception as exc: + logger.warning( + "[sandbox_attachment_sync] Sync failed: task_id=%s, attachment_id=%s, " + "error=%s", + request.task_id, + attachment_id, + exc, + ) updated.update({"status": "failed", "error": str(exc)})🤖 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 `@chat_shell/chat_shell/services/sandbox_attachment_sync.py` around lines 250 - 259, Add warning logs in both exception branches of the attachment synchronization flow, using the module’s existing logger and including task_id, attachment_id, and the caught error. Preserve the current updated status and error-field behavior, and apply this in the method containing the shown HTTPStatusError and generic Exception handlers.Sources: Coding guidelines, Linters/SAST tools
235-248: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider streaming the download instead of buffering the whole file.
response.contentholds the complete attachment in memory before the sandbox write. The upload path allows files up to 100 MB (max_upload_sizedefault inbackend/init_data/skills/sandbox/provider.py), and attachments are processed one after another, so peak memory scales with the largest attachment. Streaming withclient.stream(...)avoids the peak, if the sandbox write API accepts a file-like object.🤖 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 `@chat_shell/chat_shell/services/sandbox_attachment_sync.py` around lines 235 - 248, The attachment download in the sync flow currently buffers the entire response via response.content before sandbox.files.write. Update the request/write logic around client.get and sandbox.files.write to stream response bytes incrementally, using the sandbox write API’s supported file-like or chunked-input form, while preserving status validation and parent-directory creation.chat_shell/chat_shell/tools/sandbox/_base.py (1)
362-372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the
"sandbox"skill-name constant.The literal
"sandbox"is also defined as_SANDBOX_SKILL_NAMEinchat_shell/chat_shell/services/sandbox_attachment_sync.py. Two modules now encode the same built-in skill name. Move the constant to one shared location and import it in both places.The
hasattr(self.load_skill_tool, "get_loaded_skills")guard also hides a contract violation. If the injected object always exposesget_loaded_skills, drop the guard and keep only theis not Nonecheck.The project guideline states: "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it." As per coding guidelines.
🤖 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 `@chat_shell/chat_shell/tools/sandbox/_base.py` around lines 362 - 372, Centralize the built-in sandbox skill name in a shared module, replacing the duplicated "sandbox" literal and updating both the sandbox metadata logic and sandbox_attachment_sync.py to import the shared constant. In the surrounding load-skill metadata code, remove the hasattr check and retain only the load_skill_tool is not None guard, calling get_loaded_skills directly.Source: Coding guidelines
executor/tests/task_skills_fetch_contract.rs (1)
25-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the endpoint path into one constant.
The literal
"/api/tasks/123/skills"appears in the route, in the recorded value, and in the assertion. A future path change must update three places. Bind it once.♻️ Proposed refactor
async fn fetch_task_skills_returns_ref_metadata() { + const SKILLS_PATH: &str = "/api/tasks/123/skills"; let log = RequestLog::default();- "/api/tasks/123/skills", + SKILLS_PATH, get({ let log = log.clone(); move |headers: HeaderMap| async move { log.paths .lock() .unwrap() - .push("/api/tasks/123/skills".to_owned()); + .push(SKILLS_PATH.to_owned());- ["/api/tasks/123/skills"] + [SKILLS_PATH]Also applies to: 71-74
🤖 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 `@executor/tests/task_skills_fetch_contract.rs` around lines 25 - 32, Define a single constant for the "/api/tasks/123/skills" endpoint in the test and reuse it for the route registration, recorded log path, and assertion, including the additionally referenced lines.executor_manager/tests/services/test_sandbox_manager.py (1)
705-760: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the reuse path when skill synchronization fails.
The added tests only cover successful synchronization. The reuse branch in
executor_manager/services/sandbox/manager.py(Lines 186-192) changes the sandbox lifecycle state when_prepare_sandbox_skillsreturns an error. No test pins that behavior. Add a case wheresyncraisesSandboxSkillSyncErrorand assert the returned error and the resulting sandbox status. This test also documents the intended outcome for the concern raised onmanager.pyLine 186.🤖 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 `@executor_manager/tests/services/test_sandbox_manager.py` around lines 705 - 760, Add a pytest-asyncio case alongside test_reused_warm_sandbox_syncs_newly_loaded_skill that exercises reused-sandbox skill synchronization failure: configure _skill_synchronizer.sync to raise SandboxSkillSyncError, invoke create_sandbox with the same reuse setup, and assert the returned error plus the sandbox’s resulting lifecycle status match the behavior implemented in the reuse branch of _prepare_sandbox_skills.executor_manager/services/sandbox/manager.py (1)
301-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
synced_required_skillsmetadata. The key is only written in_start_sandbox_containerand_prepare_sandbox_skills. Remove both assignments.🤖 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 `@executor_manager/services/sandbox/manager.py` around lines 301 - 303, Remove the synced_required_skills metadata assignments from both _start_sandbox_container and _prepare_sandbox_skills, including the write shown after sandbox.set_running. Leave the remaining sandbox startup and persistence logic unchanged.executor/src/agents/runtime_capabilities.rs (1)
391-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared plan-deploy-validate sequence.
deploy_request_skills(Lines 391-409) andsync_skills_for_request(Lines 442-450) repeat the same steps: resolve the API base URL, calldeploy_skills, computemissing_required_skills, and format the samerequired Skill deployment failed: {}error. The two functions also repeat the "plan is absent" guard with the same message. Extract one helper that takes the plan and required skills and returns the report, then let each caller shape its own result.The repository guideline requires reuse: "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it."
♻️ Sketch of the shared helper
+async fn deploy_and_validate( + plan: &SkillDeploymentPlan, + request: &ExecutionRequest, + required_skills: &[String], +) -> Result<SkillDeploymentReport, String> { + let api_base_url = request_api_base_url(request); + let report = deploy_skills(plan, &api_base_url).await?; + let missing_required = missing_required_skills(required_skills, plan, &report); + if !missing_required.is_empty() { + return Err(format!( + "required Skill deployment failed: {}", + missing_required.join(", ") + )); + } + Ok(report) +}🤖 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 `@executor/src/agents/runtime_capabilities.rs` around lines 391 - 461, Extract the shared plan-deployment and required-skill validation logic from deploy_request_skills and sync_skills_for_request into one helper that accepts the deployment plan and required skill names, resolves the API base URL, calls deploy_skills, validates missing_required_skills, and returns the report or the existing failure error. Reuse this helper in both callers, while retaining each function’s existing plan-absent handling and response shaping.Source: Coding guidelines
🤖 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 `@backend/app/core/yaml_init.py`:
- Around line 471-479: Update the newer-version branch around
_is_newer_skill_version and the update_skill contract to pass the existing
Kind’s namespace and name alongside skill_id and user_id. Modify update_skill’s
Kind lookup to require and match namespace, name, and user_id, then add coverage
asserting all three identity predicates are included in the query.
In `@executor_manager/services/sandbox/manager.py`:
- Around line 186-192: In the healthy-sandbox skill synchronization branch
around _prepare_sandbox_skills, return the skill_error with the existing sandbox
without calling existing_sandbox.set_failed or persisting a FAILED lifecycle
state. Preserve the already-healthy sandbox state so subsequent create_sandbox
calls can reuse the running container and retry preparation.
- Around line 183-185: Wrap the _merge_activation_metadata call in
create_sandbox with the same try/except handling used for skill preparation,
catching SandboxSkillSyncError and returning the standard sandbox_not_ready
result instead of allowing it to escape to the E2B router. Preserve the existing
behavior for successful metadata merging and other exceptions.
In `@executor/src/agents/runtime_capabilities.rs`:
- Around line 480-493: The missing_required_skills validation resolves SKILL.md
only at skills_dir/<skill_name>, but extracted archives may preserve a different
root directory. Update the validation to locate each required skill’s SKILL.md
anywhere under plan.skills_dir while retaining the existing plan.skills and
report.failed_skills checks.
---
Nitpick comments:
In `@chat_shell/chat_shell/services/sandbox_attachment_sync.py`:
- Around line 250-259: Add warning logs in both exception branches of the
attachment synchronization flow, using the module’s existing logger and
including task_id, attachment_id, and the caught error. Preserve the current
updated status and error-field behavior, and apply this in the method containing
the shown HTTPStatusError and generic Exception handlers.
- Around line 235-248: The attachment download in the sync flow currently
buffers the entire response via response.content before sandbox.files.write.
Update the request/write logic around client.get and sandbox.files.write to
stream response bytes incrementally, using the sandbox write API’s supported
file-like or chunked-input form, while preserving status validation and
parent-directory creation.
In `@chat_shell/chat_shell/tools/sandbox/_base.py`:
- Around line 362-372: Centralize the built-in sandbox skill name in a shared
module, replacing the duplicated "sandbox" literal and updating both the sandbox
metadata logic and sandbox_attachment_sync.py to import the shared constant. In
the surrounding load-skill metadata code, remove the hasattr check and retain
only the load_skill_tool is not None guard, calling get_loaded_skills directly.
In `@executor_manager/services/sandbox/manager.py`:
- Around line 301-303: Remove the synced_required_skills metadata assignments
from both _start_sandbox_container and _prepare_sandbox_skills, including the
write shown after sandbox.set_running. Leave the remaining sandbox startup and
persistence logic unchanged.
In `@executor_manager/tests/services/test_sandbox_manager.py`:
- Around line 705-760: Add a pytest-asyncio case alongside
test_reused_warm_sandbox_syncs_newly_loaded_skill that exercises reused-sandbox
skill synchronization failure: configure _skill_synchronizer.sync to raise
SandboxSkillSyncError, invoke create_sandbox with the same reuse setup, and
assert the returned error plus the sandbox’s resulting lifecycle status match
the behavior implemented in the reuse branch of _prepare_sandbox_skills.
In `@executor/src/agents/runtime_capabilities.rs`:
- Around line 391-461: Extract the shared plan-deployment and required-skill
validation logic from deploy_request_skills and sync_skills_for_request into one
helper that accepts the deployment plan and required skill names, resolves the
API base URL, calls deploy_skills, validates missing_required_skills, and
returns the report or the existing failure error. Reuse this helper in both
callers, while retaining each function’s existing plan-absent handling and
response shaping.
In `@executor/tests/task_skills_fetch_contract.rs`:
- Around line 25-32: Define a single constant for the "/api/tasks/123/skills"
endpoint in the test and reuse it for the route registration, recorded log path,
and assertion, including the additionally referenced lines.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 707abf0b-de7e-4475-bf34-74e39a6a9f45
📒 Files selected for processing (33)
backend/app/core/yaml_init.pybackend/app/services/sandbox_file_syncer.pybackend/init_data/README.mdbackend/init_data/skills/sandbox/SKILL.mdbackend/init_data/skills/sandbox/download_attachment_tool.pybackend/init_data/skills/sandbox/provider.pybackend/tests/core/test_yaml_init_skills.pybackend/tests/init_data/skills/sandbox/test_download_attachment_tool.pybackend/tests/init_data/skills/sandbox/test_provider.pybackend/tests/services/test_sandbox_file_syncer.pychat_shell/chat_shell/services/chat_service.pychat_shell/chat_shell/services/sandbox_attachment_sync.pychat_shell/chat_shell/skills/context.pychat_shell/chat_shell/tools/sandbox/_base.pychat_shell/chat_shell/tools/skill_factory.pychat_shell/tests/test_sandbox_attachment_sync.pychat_shell/tests/test_sandbox_skill_identity.pyexecutor/src/agents/mod.rsexecutor/src/agents/runtime_capabilities.rsexecutor/src/server/mod.rsexecutor/src/services/api_client.rsexecutor/tests/agent_process_engine_contract.rsexecutor/tests/agent_runtime_capabilities_contract.rsexecutor/tests/http_contract.rsexecutor/tests/task_skills_fetch_contract.rsexecutor_manager/routers/e2b.pyexecutor_manager/services/sandbox/manager.pyexecutor_manager/services/sandbox/skill_sync.pyexecutor_manager/tests/routers/test_e2b_sandbox_creation.pyexecutor_manager/tests/services/test_sandbox_manager.pyexecutor_manager/tests/services/test_sandbox_skill_sync.pyshared/tests/utils/test_attachment_block.pyshared/utils/attachment_block.py
| elif _is_newer_skill_version(existing, metadata.get("version")): | ||
| skill_id = int(existing.metadata.labels.get("id")) | ||
| skill_kinds_service.update_skill( | ||
| db, | ||
| skill_id=skill_id, | ||
| user_id=public_user_id, | ||
| file_content=zip_content, | ||
| file_name=zip_filename, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use all required Kind identifiers during the update.
Lines 473-479 introduce an update path that calls update_skill with only skill_id and user_id. The provided implementation in backend/app/services/adapters/skill_kinds.py:677-778 then queries the Kind without namespace or name.
Extend the update service contract and its Kind query to require and match namespace, name, and user_id. Add coverage that verifies these identity predicates.
As per coding guidelines: “A Kind resource must always be queried using all three identifiers: namespace, name, and user_id.”
🤖 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 `@backend/app/core/yaml_init.py` around lines 471 - 479, Update the
newer-version branch around _is_newer_skill_version and the update_skill
contract to pass the existing Kind’s namespace and name alongside skill_id and
user_id. Modify update_skill’s Kind lookup to require and match namespace, name,
and user_id, then add coverage asserting all three identity predicates are
included in the query.
Source: Coding guidelines
| self._merge_activation_metadata( | ||
| existing_sandbox, sandbox_metadata, bot_config | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap _merge_activation_metadata in the same error handling as skill preparation.
_merge_activation_metadata calls required_skill_names on Line 338 and Line 339. required_skill_names raises SandboxSkillSyncError when required_skills is not valid JSON or is not a list. The call on Line 183 is not inside a try block. The exception therefore escapes create_sandbox and reaches the E2B router, which returns an unhandled 500 instead of the structured sandbox_not_ready response added in executor_manager/routers/e2b.py. The required_skills value arrives from the E2B request metadata, so a malformed client value triggers this path.
🐛 Proposed fix to convert the parse failure into the standard error result
- self._merge_activation_metadata(
- existing_sandbox, sandbox_metadata, bot_config
- )
+ try:
+ self._merge_activation_metadata(
+ existing_sandbox, sandbox_metadata, bot_config
+ )
+ except SandboxSkillSyncError as exc:
+ return existing_sandbox, str(exc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._merge_activation_metadata( | |
| existing_sandbox, sandbox_metadata, bot_config | |
| ) | |
| try: | |
| self._merge_activation_metadata( | |
| existing_sandbox, sandbox_metadata, bot_config | |
| ) | |
| except SandboxSkillSyncError as exc: | |
| return existing_sandbox, str(exc) |
🤖 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 `@executor_manager/services/sandbox/manager.py` around lines 183 - 185, Wrap
the _merge_activation_metadata call in create_sandbox with the same try/except
handling used for skill preparation, catching SandboxSkillSyncError and
returning the standard sandbox_not_ready result instead of allowing it to escape
to the E2B router. Preserve the existing behavior for successful metadata
merging and other exceptions.
| skill_error = await self._prepare_sandbox_skills( | ||
| existing_sandbox, existing_sandbox.base_url | ||
| ) | ||
| if skill_error: | ||
| existing_sandbox.set_failed(skill_error) | ||
| self._repository.save_sandbox(existing_sandbox) | ||
| return existing_sandbox, skill_error |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not mark a healthy sandbox as FAILED when only skill synchronization fails.
This branch runs after the health check passed, so the container is alive. A transient skill-sync failure (backend timeout, executor 5xx) now calls set_failed and persists that state. is_active() then returns False for the sandbox. The next create_sandbox call for the same task_id skips the reuse branch and provisions a new sandbox without calling _cleanup_dead_sandbox first. A recoverable preparation error therefore destroys a working runtime.
Return the error without changing the persisted lifecycle state, so the caller retries against the same healthy container.
🛠️ Proposed fix to keep the running sandbox usable
skill_error = await self._prepare_sandbox_skills(
existing_sandbox, existing_sandbox.base_url
)
if skill_error:
- existing_sandbox.set_failed(skill_error)
self._repository.save_sandbox(existing_sandbox)
return existing_sandbox, skill_error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| skill_error = await self._prepare_sandbox_skills( | |
| existing_sandbox, existing_sandbox.base_url | |
| ) | |
| if skill_error: | |
| existing_sandbox.set_failed(skill_error) | |
| self._repository.save_sandbox(existing_sandbox) | |
| return existing_sandbox, skill_error | |
| skill_error = await self._prepare_sandbox_skills( | |
| existing_sandbox, existing_sandbox.base_url | |
| ) | |
| if skill_error: | |
| self._repository.save_sandbox(existing_sandbox) | |
| return existing_sandbox, skill_error |
🤖 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 `@executor_manager/services/sandbox/manager.py` around lines 186 - 192, In the
healthy-sandbox skill synchronization branch around _prepare_sandbox_skills,
return the skill_error with the existing sandbox without calling
existing_sandbox.set_failed or persisting a FAILED lifecycle state. Preserve the
already-healthy sandbox state so subsequent create_sandbox calls can reuse the
running container and retry preparation.
| fn missing_required_skills( | ||
| required_skills: &[String], | ||
| plan: &SkillDeploymentPlan, | ||
| report: &SkillDeploymentReport, | ||
| ) -> Vec<String> { | ||
| required_skills | ||
| .iter() | ||
| .filter(|skill| { | ||
| !plan.skills.contains(skill) | ||
| || report.failed_skills.contains(skill) | ||
| || !plan.skills_dir.join(skill).join("SKILL.md").is_file() | ||
| }) | ||
| .cloned() | ||
| .collect() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how extract_skill_zip maps archive entries to the skill directory.
rg -n -A40 'fn extract_skill_zip' executor/src/agents/runtime_capabilities.rsRepository: wecode-ai/Wegent
Length of output: 2189
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- runtime capability symbols ---'
rg -n -A35 -B20 'extract_skill_zip|missing_required_skills|SkillDeploymentPlan|SkillDeploymentReport|skills_dir' executor/src/agents/runtime_capabilities.rs
printf '%s\n' '--- skill archive producers and response handling ---'
rg -n -S -A25 -B15 'skill.*zip|SKILL\.md|archive|skills_dir|SkillDeployment' executor/src | head -n 500
printf '%s\n' '--- repository skill packaging references ---'
rg -n -S -A12 -B12 'SKILL\.md|skill.*archive|skill.*zip|skills/' --glob '!target/**' --glob '!node_modules/**' . | head -n 500Repository: wecode-ai/Wegent
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- download endpoint implementations ---'
rg -l -S 'kinds/skills|download.*skill|Skill.*download|skills.*download' --glob '!target/**' --glob '!node_modules/**' . | head -n 100
printf '%s\n' '--- ZIP creation and skill package paths ---'
rg -l -S 'ZipWriter|zip::|SKILL\.md|skill_store_path|SkillSyncSpec' --glob '!target/**' --glob '!node_modules/**' . | head -n 100
printf '%s\n' '--- focused backend matches ---'
rg -n -S -A30 -B20 'kinds/skills|ZipWriter|skill_store_path|SkillSyncSpec' \
--glob '!target/**' --glob '!node_modules/**' . | head -n 700Repository: wecode-ai/Wegent
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- backend skill upload/download handlers ---'
sed -n '1,260p' backend/app/api/endpoints/kind/skills.py
sed -n '1,260p' backend/app/services/skill_service.py
printf '%s\n' '--- backend skill package validation and storage ---'
rg -n -S -A35 -B20 'zipfile|ZipFile|SKILL\.md|content.*file|archive|package' \
backend/app/api/endpoints/kind/skills.py backend/app/services/skill_service.py \
backend/app/services/adapters/skill_kinds.py backend/app/schemas/kind.py
printf '%s\n' '--- focused archive contract tests ---'
rg -n -S -A25 -B15 'zip|SKILL\.md|download|upload|root|directory' \
backend/tests/api/test_kind_skill_router.py backend/tests/api/test_skills_api.py \
backend/tests/services/test_skill_service.py backend/tests/api/test_skill_download_transactions.py \
executor/tests/skill_downloader_contract.rs executor/tests/agent_runtime_capabilities_contract.rsRepository: wecode-ai/Wegent
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from io import BytesIO
from pathlib import PurePosixPath
from zipfile import ZIP_DEFLATED, ZipFile
requested = "api-test-skill"
archive_root = "test"
payload = BytesIO()
with ZipFile(payload, "w", ZIP_DEFLATED) as archive:
archive.writestr(f"{archive_root}/SKILL.md", "# Skill")
extracted = [PurePosixPath(name) for name in ZipFile(BytesIO(payload.getvalue())).namelist()]
installed_path = PurePosixPath("skills", requested, "SKILL.md")
archive_path = PurePosixPath("skills", *extracted[0].parts)
print(f"archive entry: {extracted[0]}")
print(f"extracted path: {archive_path}")
print(f"required path: {installed_path}")
print(f"required path exists: {archive_path == installed_path}")
assert archive_path != installed_path
PYRepository: wecode-ai/Wegent
Length of output: 295
Extract each skill archive under skills_dir/<skill_name>. The backend preserves the uploaded archive root. An archive containing test/SKILL.md for api-test-skill extracts to skills_dir/test/SKILL.md, so missing_required_skills rejects the deployment.
🤖 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 `@executor/src/agents/runtime_capabilities.rs` around lines 480 - 493, The
missing_required_skills validation resolves SKILL.md only at
skills_dir/<skill_name>, but extracted archives may preserve a different root
directory. Update the validation to locate each required skill’s SKILL.md
anywhere under plan.skills_dir while retaining the existing plan.skills and
report.failed_skills checks.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes