Repository navigation
Conversation
zakahan
left a comment
There was a problem hiding this comment.
This is by an automated agent, not a human.
Reviewed head 30ea7f1. The implementation matches the description: the AIO toolset binds one AgentKit Session per ADK app/user/session, discovers the catalog through GET /v1/skills, reads instructions lazily, routes the five tools to the same sandbox, forwards neither the host environment nor local fallbacks, and bounds output with remote kill/close on timeout and cancellation. No blocking findings; two minor items are inline.
Verification
- Reviewed the complete 9-file diff (
veadk/agent.py,check_skills_callback.py,skills_toolset.py, newaio_sandbox.py, tests, paired docs and the two PRD files) at head30ea7f12abf0f13d57435e238122e6f849edd00d. - Ran
uv run --extra dev pytest tests/skills/test_aio_sandbox_skills.py -qat the reviewed head: 19 passed. gh pr checksat head: all green (test 3.10/3.12, pre-commit, gitleaks, codex-smoke, dependency-policy, harness gates). PR isMERGEABLEandBLOCKEDpending approval.- Not reproduced: the live
doubao-seed-evolvingsandbox acceptance and the docs type-check/build reported in the PR description. - Residual risks: an expired or deleted bound sandbox fails the turn by design (no rebinding or local fallback),
AGENTKIT_TOOL_IDis required at the first turn, and the AIOskillscontract is a breaking change for callers that previously passed local paths or cloud SkillSpace IDs (documented in the PR).
| @@ -0,0 +1,48 @@ | |||
| # AIO sandbox skills and basic tools | |||
There was a problem hiding this comment.
[Minor] This PR was opened against a base whose AGENTS.md required a bilingual PRD under prd-spec/features/, and the two files comply with that base policy. Current main no longer has prd-spec/ and its AGENTS.md now says not to create standalone PRD/component-spec files (unless explicitly requested), so merging this as-is re-introduces the removed directory. Drop the paired PRD files when rebasing, unless maintainers explicitly want the record; the PR description already carries intent, verification and limits.
|
|
||
|
|
||
| def validate_paths(paths): | ||
| if any(not path.startswith("/") or "\0" in path for path in paths): |
There was a problem hiding this comment.
[Minor] validate_paths only rejects non-absolute paths and NUL bytes. Values such as /workspace/skills/../.., a doubled slash, duplicate entries or a bare / pass and are forwarded as repeated path query parameters, so the catalog scope is whatever the server normalizes rather than the configured roots. The values are caller config, so the risk is low, but posixpath.normpath plus rejecting empty/duplicate normalized entries would make the scope explicit.
Agent(skills_mode="aio_sandbox", skills=[...])now uses the existingSkillsToolsetto load Skill instructions and execute bash/read/write/edit inside a session-bound AgentKit sandbox. Empty skills use the sandbox default catalog; multiple directories are passed toGET /v1/skillsas repeatedpathparameters.The ADK app/user/session identity determines the binding, which is reused across turns. Construction does not read host Skill files, commands do not inherit host environment variables, and failed/expired bindings never fall back to local execution. Remote tools handle timeout, cancellation and shell cleanup without automatically retrying mutations. Existing
localandskills_sandboxbehavior is preserved; AIO callers using cloud SkillSpace IDs must switch to sandbox directories.Validation:
23014771).doubao-seed-evolvingverified the sandbox Tool ID, host environment isolation, all five tools, and sandbox reuse across turns.c662cc8dstopped with 3409 passed, 8 skipped and 6 failures: three migration-delivery tests lackedzip, and three Harness resource tests lackedllama_index. Full regression is not claimed green.Includes paired English/Chinese usage documentation and a concise PRD. No dependency changes.