mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-24 21:46:17 +00:00
101 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Aari
|
0f7d8709d3
|
feat(sandbox): add controlled egress with approvals (#5152)
* feat(sandbox): add controlled egress approvals * Apply batched suggestions from code review * fix(sandbox): harden restricted network policy * fix(sandbox): harden denied egress handling * fix(sandbox): isolate network proxy sidecar * chore: retry sandbox image smoke * fix(sandbox): close remaining network policy gaps * fix(sandbox): harden relay token rejection * fix(sandbox): fence incompatible policy replacement * fix(sandbox): replace containers across network modes * fix(sandbox): close remaining lifecycle gaps --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Willem Jiang
|
037658b0ee
|
fix(ci):reduce the size of AGENTS.md in sandbox (#5146) | ||
|
gus
|
47f43f79f4
|
fix: improve local environment detection guidance (#5111)
Co-authored-by: angus-guo <217034332+angus-guo@users.noreply.github.com> |
||
|
Aari
|
9e0fbd60fa
|
fix(sandbox): isolate concurrent subagent shell sessions (#5134)
* fix(sandbox): isolate concurrent subagent shell sessions * fix(sandbox): make execution acquire idempotent * fix(sandbox): close execution lifecycle gaps * fix(sandbox): serialize retained client lifecycle * fix(sandbox): close remaining client lifecycle gaps * fix(sandbox): unwind failed client lookup * fix(sandbox): protect internal lease identities * fix(sandbox): make cancellation reconciliation durable * fix(sandbox): fence cancelled workers and IM uploads |
||
|
nicochow
|
bbcfd368bf
|
fix(sandbox): scrub SSH_AUTH_SOCK from the sandbox subprocess env (#5145)
SSH_AUTH_SOCK points at the host's ssh-agent socket. A sandbox subprocess that inherits it can sign and authenticate with every key the agent holds (git push, ssh logins) without reading any key file -- the same credential-pointer leak class as the *_ASKPASS helpers the env policy already scrubs deliberately. No wildcard pattern fits (*AUTH* would strip benign names), so add an exact entry to _BLOCKED_EXACT_NAMES. A skill that genuinely needs the agent socket can still declare it via required-secrets: injected values win over the blocklist by design. Co-authored-by: zhouyujie <zhouyujie@keep.com> |
||
|
Zeren Wang
|
a06a6fed7e
|
feat(harness): deterministic acceptance checklist for subagent delegations (RFC #4651, layer 2) (#5109)
* feat(harness): deterministic acceptance checklist for subagent delegations (RFC #4651, layer 2) PR4 of RFC #4651: check lead-supplied acceptance_criteria in code when a subagent completes, so objectively checkable requirements can never be silently passed by a self-report. - subagents/acceptance_checks.py: deterministic leaf families — file:<path> exists|non-empty and file_written:<path> read through read_current_file_content scoped to the shared thread workspace; the read uses the sandbox-native virtual path form (the local read validator and provider mount tables resolve /mnt/user-data/... paths, not host paths); the scope decision canonicalizes with realpath on the local sandbox so workspace symlinks cannot escape into uploads; a remote provider's "Error: ..." return string is normalized to a failed check (provider-typed via is_local_sandbox); a UnicodeDecodeError marks a binary deliverable as existing and non-empty; out-of-scope paths degrade to UNVERIFIED. tests_passed:<command> anchors to a matching recorded bash execution with status=success and a test-summary shape; matching is shell-structure aware with control-flow attribution (span must end at the last segment with provable execution), negating-option values are ineligible evidence and a target negated anywhere in the command degrades the match, extra flags must be selection-preserving, extra positionals widen only after a path-scoped criterion, truncated commands degrade via command_truncated, the summary shape is read only from output attributable to the matched segment (preceding segments provably silent by invocation form), and pass shapes require a nonzero passed count. Criterion text is neutralized with neutralize_untrusted_tags before storage/rendering. Anything else renders UNVERIFIED, never silently passed. - executor: accumulate bounded bash command/output evidence per streamed chunk (merged by tool_call_id, newest-capped) so subagent summarization compacting earlier messages cannot erase a recorded execution; the recorded status is the actual shell exit status parsed from the output's exit marker (signed codes included; the remote Command exited with code N form is accepted only as the whole trimmed output), falling back to deerflow_tool_meta only when no marker exists. - sandbox providers: e2b/opensandbox/tenki/boxlite append the LocalSandbox-style "Exit Code: N" marker on nonzero exit even with non-empty output; aio propagates the SDK's structured exit_code on both exec paths the same way; local timeouts append Exit Code: 124; and _truncate_bash_output always preserves a trailing exit marker (signed included) inside its budget, with a 32-char floor raising any smaller configured limit, so the actual shell outcome always survives in the output text. - task_tool: run the checklist offloaded (asyncio.to_thread) on the completed branch, failure-isolated; stamp the verdict into result metadata and render the per-criterion section into the model-visible result text. - status contract: additive subagent_acceptance_verdict transport with read-side structural validation. - delegation ledger: entry carries the verdict and renders a compact acceptance segment; gateway strips caller-forged verdicts from both ledger entries and message metadata, like the citation verdict. - blocking-IO anchor pins the offload (teeth proven red->green); leaf read errors catch only OSError/SandboxError so unexpected errors reach the task-tool-level isolation instead of being mislabeled. * fix(harness): close acceptance evidence gaps from review (RFC #4651 PR4) - negating options: overlap with a matched criterion target is now checked by path/nodeid prefix, not exact token equality — excluding a sub-path of the criterion's selection (pytest tests --deselect tests/unit/test_auth.py) degrades to UNVERIFIED instead of holds - output attribution: any redirection token in the matched final segment makes the recorded tail non-attributable (> / >> / 2> are word characters to the parser, so redirection was invisible to the matcher) - silent-source allowlist narrowed from any *activate suffix to the */bin/activate shape - status_contract docstring: restore the shared-fixture sentence and note subagent_acceptance_verdict is deliberately outside the fixture - executor: update_bash_executions publishes [] (stream carried no bash-family calls) instead of collapsing it into None, mirroring update_tool_receipts * fix(harness): close acceptance residual gaps from re-review (RFC #4651 PR4) - tests_passed: add error outcomes to the fail shapes — "4 passed, 1 error" and pytest's "ERROR <nodeid>" short summary no longer satisfy the pass shape when the exit status is swallowed (|| true) or absent; zero-error counts stay clean. - file leaves: bound the deliverable read — a "wc -c" shell size probe answers files above 50k bytes without loading ~2x their size, honoring the host-bash kill switch and falling back to the full read on any non-integer rendering, so verdicts never get less sound. - executor: record the exit marker text as status_marker on harvested bash evidence; the leaf detail now reports the marker actually seen instead of asserting a failure indistinguishable from the command's own trailing text. - extend the blocking-IO anchor to drive the probe branch inside the offload; teeth re-verified red->green. * fix(harness): close acceptance forgery and bound gaps from P2 re-review (RFC #4651 PR4) - file leaves: never read unbounded — size is established first (os.stat on the validated local host path, so the host-bash-disabled configuration needs no shell; a guarded wc -c on remote providers that renders missing/unreadable in its own words). Above the 50k cap the leaf answers from the size alone, at/below it the full read runs, and an unestablishable size degrades to UNVERIFIED instead of an unlimited fallback read. - output attribution: source/. prefixes are never provably silent — a crafted */bin/activate path shape says nothing about what the script prints, so sourced segments can no longer lend a passing summary. - executable identity: an explicitly path-spelled criterion now requires the same normalized executable path; the basename rule stays only for deliberately bare criterion commands. * fix(harness): run acceptance size probe outside subagent-controlled state (RFC #4651 PR4) - remote probe no longer runs in the sandbox's persistent shell: a fresh env -i /bin/sh with absolute-path stat/realpath (poisoned functions, aliases, PATH, exported functions, IFS, locale cannot steer it), plus a marker env routing AIO onto a fresh per-call bash.exec session. - metadata-only: stat never opens content, so a FIFO deliverable cannot block the parent for the provider's idle timeout; non-regular files (fifo/dir/symlink) degrade to UNVERIFIED. - containment canonicalized against the literal mount root: a final-component symlink or a swapped parent directory (root included) cannot redirect the check outside shared storage; unprovable layouts degrade to UNVERIFIED. * fix(harness): canonicalize probe containment against the canonical mount root (RFC #4651 PR4) Literal-root equality made every remote file leaf permanently UNVERIFIED on e2b and Tenki, which realize /mnt/user-data as a symlink to the home dir by default (e2b bootstrap 'sudo ln -sfn', Tenki best-effort symlink). Containment now compares the file's realpath against the mount root's realpath — exactly what the provider's own read path resolves, so probe and read-back stay consistent; final-component symlinks stay rejected by the non-dereferencing stat, and an intermediate dir-link escape under a sane root still lands ESCAPED. The inner script is a module constant and the suite now executes the composed probe for real against on-disk layouts (real dir, symlinked prefix, final symlink, fifo, missing, dir-link escape), which the canned-output stub could not see. * fix(harness): close bare-criterion negation and CDPATH summary channels (RFC #4651 PR4) - matching: a criterion with no positional selection target (bare pytest, make test) stands for the runner's default selection, so ANY negating option (--ignore/--deselect/...) makes the recorded run a different selection — unprovable. The overlap guard only sees consumed criterion tokens, which a bare criterion does not have; scoped criteria keep the unrelated-exclusion behavior. - attribution: cd is no longer blanket-silent — CDPATH makes cd print the resolved (subagent-chosen) destination and the pass shapes match as substrings, so one mkdir 'all tests passed' plus an export minted a pass for any quiet command. A cd argument or CDPATH= value (export or leading assignment) carrying any summary shape makes the segment non-silent; shape-free cd dir wrappers keep matching. - docs: _truncate_bash_output states the effective 32-char floor (the guarantee previously read as an unconditional max_chars bound). * fix(harness): close env-assignment and expansion channels in acceptance matching (RFC #4651 PR4) Self-audit in the shape of the last review rounds — channels the matcher classified as accounted-for that can change what runs, narrow the selection, or lend the summary text: - env assignments are no longer blanket-stripped: only an allowlist of inert display/CI knobs (CI, NO_COLOR, PY_COLORS, ...) may prefix a matched span, and a non-allowlisted assignment in any preceding segment (pure-assignment or export NAME=) is state pollution — PATH redirects the executable, LD_PRELOAD/PYTHONPATH/NODE_OPTIONS inject code, PYTEST_ADDOPTS/GOFLAGS/MAKEFILES inject selection-changing inputs, BASH_ENV runs arbitrary shell startup. All degrade to unprovable. - runtime expansions: any span token carrying /$( )/backticks, any negating-option value carrying an expansion or glob (unknown excluded set), and any extra executed token carrying glob metacharacters (crafted option-looking filenames narrow invisibly) are unprovable. Criterion-side globs stay self-consistent (literal match). - cd: an argument carrying a runtime expansion or glob is non-silent (unknown destination, unknown print); CDPATH= assignments are now handled as state pollution at the match layer, subsuming the value-shape special case. * fix(harness): persistent-shell evidence, exact env sets, option-arity scoping (RFC #4651 PR4) - tests_passed: on a persistent-shell provider (new Sandbox.persistent_shell_sessions capability, set by AioSandbox) every leaf degrades to UNVERIFIED — any earlier call in the shared session could have mutated the state the clean-looking run executed in, and only a fresh controlled session (RFC section 6 verifier) can prove otherwise. The flag is read from the provider registry without acquiring a sandbox. - env assignments: the allowlist is gone — no variable is provably inert across repositories (CI/DEBUG are routinely read by tests). The span's assignment prefix must equal the criterion's exactly (values included, order-insensitive); any assignment or export NAME= in a preceding segment is state pollution. - scoping: positional targets are now read by option arity, so a path embedded in an option (--basetemp=/tmp/p, --junitxml=/tmp/r.xml) never counts as a selection target and an extra positional after such a criterion narrows the default selection it denotes. * fix(harness): stamp shell provenance at harvest, close export/unset and arity gaps (RFC #4651 PR4) * fix(harness): split physical newlines as shell separators in acceptance matching (RFC #4651 PR4) * fix(harness): scope cd wrappers to thread data roots, pin accepted boundaries (RFC #4651 PR4) * fix(harness): preserve criterion connectors, prove file_written readable, fail-closed shell capability (RFC #4651 PR4) * fix(harness): compare only the connector prefix, tolerate trailing criterion semicolons (RFC #4651 PR4) * fix(harness): preserve continuation-line operators, keep ./-spelled executable identity (RFC #4651 PR4) * fix(harness): render criteria single-line so a multiline criterion cannot inject a forged checklist line (RFC #4651 PR4) * fix(harness): reject parent-traversal executable tokens in acceptance matching (RFC #4651 PR4) * fix(harness): reject parent-traversal negated values in acceptance matching (RFC #4651 PR4) |
||
|
Aari
|
cdc886ae85
|
fix(sandbox): make tool descriptions optional (#4878)
* fix(sandbox): make tool descriptions optional * fix(sandbox): address optional description review * test(sandbox): pin optional description contracts |
||
|
Janlay
|
45adb8fbb5
|
perf(runtime): bound gateway memory after terminal runs (#5112)
* fix(runtime): clean up terminal run records * perf(sandbox): bound local path caches * perf(runtime): release terminal run cycles * fix(runtime): address terminal cleanup review * fix(runtime): clean up after end publish failure * fix(runtime): guard terminal cleanup from cancellation * fix(runtime): discard fenced journal buffers * fix(runtime): harden abort and teardown paths |
||
|
Aari
|
317577e285
|
fix: enforce custom agent skill allowlists in sandboxes (#5077)
* fix: enforce agent skill allowlists in sandboxes * fix: guard E2B skill projection resets * fix: preserve agent skill isolation across delegation * fix: close sandbox skill isolation bypasses * fix(sandbox): close skill isolation review gaps * fix(sandbox): harden skill isolation lifecycle |
||
|
PeaceMaker-best
|
137a3cb60d
|
fix(authz): recheck policy before sandbox reuse (#5006)
* fix(authz): recheck policy before sandbox reuse * fix(authz): avoid duplicate async sandbox checks * fix(authz): scope sandbox decision across middleware * fix(authz): construct async providers on the event loop * test(authz): avoid cold imports under Blockbuster --------- Co-authored-by: 嗜鵼 <hy2010hy2010@qq.com> Co-authored-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Wu Shuwen
|
a4e2a2b934
|
fix(sandbox): bound Windows command execution (#4946)
* fix(sandbox): bound Windows command execution * fix(sandbox): preserve Windows output encoding * fix(sandbox): honor Python UTF-8 mode * fix(sandbox): normalize captured command newlines * fix(sandbox): scope newline normalization to Windows |
||
|
Aniket Wagh
|
8a830f6354
|
fix(deps): depend on renamed tenki package instead of tenki-sandbox (#5087)
* fix(deps): depend on renamed tenki package instead of tenki-sandbox tenki-sandbox has been removed from PyPI and republished as tenki. Its old wheel URL still resolves, so existing lockfiles keep installing and the breakage is invisible to anyone with a warm lock; any fresh resolution fails with 'tenki-sandbox was not found in the package registry'. tenki 1.0.2 still ships the tenki_sandbox module, so the imports in community/tenki/provider.py and sandbox.py are unchanged. Fixes #5081 * fix(tenki): point install guidance at the renamed distribution The rename to `tenki` left the user-facing remediation still naming the removed package. `_import_client` raised "pip install tenki-sandbox" on the missing-extra path — the exact instruction this change proves now 404s on PyPI, handed to the user at the exact moment they need it to work. Update that message and the remaining `tenki-sandbox` references in the provider, sandbox adapter, README, sandbox AGENTS.md and the test docstring. The imported module stays `tenki_sandbox`, so the distribution and module names now differ; each mention says so rather than just swapping the string. No behavior change beyond the error text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tenki): migrate the provider to the 1.x workspace-only API Renaming the dependency was not enough. tenki 1.0.2 keeps the tenki_sandbox module name but not its contract: Client.create dropped project_id and has no **kwargs to absorb it, and IdentityWorkspace no longer carries `projects` (the attribute is gone from the package entirely). Both configuration paths therefore failed before a sandbox could be created — explicit project scope raised TypeError, and automatic scope raised AttributeError walking workspace.projects. Scope is now the workspace alone. _resolve_scope returns a single workspace id, auto-selecting when the account has exactly one, and project_id is gone from create_kwargs and from the documented config surface. A stale project_id in config.yaml warns rather than fails. SandboxConfig is extra="allow", so simply not reading the key would leave it scoping nothing with no signal; it also used to short-circuit the identity lookup, so operators with more than one workspace need to know they must now set workspace_id. The suite passed against the broken provider because the fake client took **kwargs and swallowed the project_id the real SDK rejects. The double now mirrors 1.0.2 — keyword-only, no **kwargs — so an unexpected argument is a TypeError in tests exactly as it is against the SDK. Reintroducing the old create call fails 20 tests; before this change it failed none. Verified against the exact locked wheels: every other kwarg the provider passes (name, workspace_id, sticky, wait, max_duration, image, cpu_cores, memory_mb, env) and every SDK surface it touches (who_am_i, Identity.workspaces, wait_ready, exec, close, the fs API, the four terminal exception classes) is unchanged in 1.0.2. Reported by willem-bd in review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(config): drop sandbox.project_id from the Tenki example The canonical example still documented project_id as a supported optional key after the provider stopped honouring it, so an operator following it could set the key, get no scope from it, and hit a workspace-resolution failure with nothing in the example to explain why. Replaced with a migration note rather than a silent deletion: someone upgrading already has the key in their config.yaml and needs to know it is inert now and that workspace_id is what scopes a sandbox on Tenki 1.x. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Aniket Wagh <aniketwaghh@users.noreply.github.com> |
||
|
Zeren Wang
|
bb75f8d736
|
feat(sandbox): share sandbox identity derivation and acquire serialization (#4741) (#5089)
* feat(sandbox): share sandbox identity derivation and acquire serialization (#4741) Remote providers (AIO, E2B, BoxLite, Tenki, OpenSandbox) each inlined the same sha256(user:thread)[:16] sandbox-id expression and kept per-scope lock dicts that grew unboundedly until shutdown. This extracts both mechanisms into shared components without changing provider lifecycle, ids, capacity semantics, or public tool behavior: - sandbox/identity.py: keyword-only derive_sandbox_scope_token (byte-pinned compatibility contract) + is_sandbox_scope_token; per-provider golden vectors pin current behavior including BoxLite's raw-None quirk and each provider's private user_id resolution. - sandbox/acquire_serialization.py: AcquireSerializer — per-key lock table with holder/waiter refcount reclamation, bounded dedicated executor (async waits off both the event loop and the default executor), worker-owned cancellation cleanup (no event-loop callback dependency), idempotent close(). - Each provider adopts both components; AIO/E2B key by (user_id, thread_id) with acquire and (E2B) release serialized; BoxLite/Tenki/OpenSandbox key by derived sandbox id and offload the whole sync acquire to the serializer's executor so a cancelled awaiter cannot overlap a retried same-scope body (leaked-remote-VM regression caught in review). - thread_id=None acquires stay unserialized; provider shutdown()/reset() close the serializer; E2B capacity/ledger/reconciliation and AIO ownership/flock machinery untouched. - blocking-IO anchor proves contended OpenSandbox acquire_async stays off the event loop (teeth verified red/green); AGENTS.md documents the shared components. * refactor(sandbox): address review on acquire serialization (#5089) - Replace unreachable checkin branch with an assertion: run() returns False only after abandon(), which the except handler always re-raises; the old _checkin would have double-decremented the refcount. - Document the task.cancelling() == 0 assumption in hold_async. - Drop unused thread_id/user_id kwargs from BoxLite and Tenki _acquire_scope_locked (OpenSandbox still forwards them). * fix(sandbox): preserve request ContextVars in acquire executor bridge (#5089) loop.run_in_executor() does not copy contextvars, unlike the inherited SandboxProvider.acquire_async() which used asyncio.to_thread(). The BoxLite/OpenSandbox/Tenki acquire_async bridges introduced in this PR therefore dropped the request trace id (logged as trace_id=-). Add AcquireSerializer.run_on_executor(), which copies the calling context and runs the callable through ctx.run, and route all three providers through it. Add regression tests binding request_trace_context and verifying the worker thread observes it. |
||
|
georgelichen
|
1c219b6864
|
fix(sandbox): scope MSYS path conversion exclusions (#5003)
* Preserve Windows CLI compatibility for local sandbox commands MSYS path conversion must remain disabled for DeerFlow virtual paths, but applying a blanket environment override to every POSIX command breaks host-native CLI shims on Windows. Limit MSYS argument-conversion exclusions to safe non-root virtual path prefixes, omit values that would broaden the exclusion pattern, and document the contract. Constraint: Preserve the virtual-path protection introduced by #2765/#2766 Rejected: Disable MSYS conversion for every command | breaks Windows CLI shims Rejected: Toggle blanket conversion only for commands containing virtual paths | host CLIs can receive virtual-path arguments and still need normal conversion for their own paths Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep regression coverage for virtual-path arguments, root mounts, and host-native CLI launchers Tested: test_local_sandbox_encoding.py (12 passed); related sandbox suite (197 passed, 8 skipped, 7 failures matching origin/main); ruff check; ruff format --check; git diff --check; direct LocalSandbox CLI and virtual-path smoke tests Not-tested: Full offline suite completion; stopped at 6% after unrelated Windows and optional-runtime failures Related: #2765 Related: #2766 * Keep MSYS regression tests portable across CI operating systems The Windows-shell environment tests patched os.name to nt while mounting Windows-specific paths. On Linux and macOS, pathlib then attempted to construct WindowsPath during command resolution or output masking, so the backend merge gate failed before exercising the environment contract. Stub the exclusion boundary in execute-command tests and retain mapping-specific filtering coverage in the helper test. Constraint: Backend unit tests run on Linux, while the behavior under test is Windows-only Rejected: Skip the tests outside Windows | would remove CI coverage of the environment contract Rejected: Patch pathlib internals | couples tests to implementation details and hides the platform boundary Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep OS-specific subprocess assertions independent from host-path resolution Tested: test_local_sandbox_encoding.py (12 passed); ruff check; ruff format --check; git diff --check Not-tested: Linux runner execution locally because Docker Desktop is unavailable and WSL cannot access this linked worktree Related: #5003 Related: https://github.com/bytedance/deer-flow/pullrequestreview-5013380238 |
||
|
hataa
|
cc6a2657e7
|
feat(authz): enforce sandbox:execute authorization at sandbox acquisition (#4063 Phase 3) (#4911)
Sandbox is an execution environment, not a named resource: multiple tools
(bash, read_file, write_file, glob, grep, ...) depend on it, all funneled
through ensure_sandbox_initialized / ensure_sandbox_initialized_async. Gate
the single acquisition entry point (single source of truth) instead of
maintaining a sandbox-tool-name set in middleware:
- authorize_sandbox_execution helper (authz/sandbox_authz.py) checks
authorize("sandbox", "execute", target="*") — a binary judgment
(can this role use the sandbox at all); RBAC allow:"*"/true permits,
allow:[]/false denies.
- lazy path: ensure_sandbox_initialized (+ async) calls the gate before
provider.acquire.
- eager path: SandboxMiddleware.before_agent / abefore_agent call the gate
before _acquire_sandbox.
- deny raises SandboxAuthorizationError (SandboxError subclass) which
propagates through tool execution as a friendly ToolMessage (RFC §9:
'not a crash').
- authorization.enabled: false is a no-op everywhere; provider errors
follow fail_closed (deny) / fail_open (allow).
12 tests in tests/test_sandbox_authorization.py cover disabled/allow/deny/
deny-via-bool/no-policy-unrestricted/provider-error-fail-closed/open/
internal-caller + ensure_sandbox_initialized deny (never acquires) and
allow (acquires) integration paths.
|
||
|
Ryker_Feng
|
ccff5f5ce7
|
docs: govern agent guidance size (#4799)
* docs: govern agent guidance size * refactor: split agent guidance by code scope * Clarify virtual path handling in AGENTS.md Updated the translation section to clarify the role of `LocalSandboxProvider` and the handling of virtual paths in the tool layer. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Xinmin Zeng
|
42fd5aa0b5
|
fix(sandbox): keep skill reads on provider mappings (#4792) | ||
|
Xinmin Zeng
|
f2e832330e
|
fix(sandbox): enforce disabled skills in filesystem views (#4178)
* fix(sandbox): project enabled skills into sandbox views * fix(skills): keep projection mutations consistent * fix(skills): fail closed on projection errors * fix(skills): isolate per-scope failures during boot projection rebuild rebuild_all_skill_projections() propagated any exception from the public rebuild or from a single user's rebuild straight out of the gateway lifespan startup, uncaught. A single broken user directory (bad permissions, corrupted _skill_states.json, unreadable content) would therefore abort gateway boot for every user, not just that one - _rebuild_*_locked already fails closed internally (clears the view and re-raises), so the boot loop only needed to stop treating that re-raise as fatal. Each scope's rebuild now fails closed independently and boot continues; a scope left empty by a boot failure self-heals on the next sandbox acquire via ensure_skill_projections(). Also patches deerflow.skills.projection.rebuild_all_skill_projections in the memory-flush lifespan test fixture, matching the two sibling fixtures in the same file — this call is now on the lifespan startup path and the fixture's minimal SimpleNamespace config predates it. * test(skills): update authz test for the projection-aware public toggle _persist_shared_skill_state (introduced earlier in this branch) reads the shared extensions_config.json fresh from disk under the projection lock instead of through the cached get_extensions_config() singleton - that's the whole point of the fix (stale worker caches must not clobber another worker's concurrent update). The name no longer exists on the skills router module, so the test's monkeypatch of it started raising AttributeError instead of exercising the endpoint. The mock storage in this test isn't a real LocalSkillStorage instance, so _persist_shared_skill_state's projection-mutation branch is already skipped (nullcontext) and it falls back to a fresh ExtensionsConfig() for the nonexistent tmp config_path - no replacement monkeypatch needed. * fix(sandbox): make skill projection ensure best-effort in acquire acquire() called _ensure_skills_projection() directly, outside any try/except, in both LocalSandboxProvider and AioSandboxProvider. Every other skill-mount setup path in these providers has always caught exceptions and logged a warning rather than failing sandbox acquire outright (e.g. when config.yaml can't be resolved) - these two new call sites broke that contract, so any projection failure (including simply not having a config.yaml, as in CI's test environment) now failed acquire() itself instead of just leaving skill mounts off. _ensure_skills_projection now catches its own exceptions and returns None; both providers' callers already tolerate that (a None projection skips the skill-specific mounts, matching the existing degrade path) after making _append_public_skill_mapping and the custom/legacy mount block in LocalSandboxProvider explicitly None-safe. Caught by running the full suite with config.yaml removed, matching CI's environment - not caught locally because a real config.yaml was present, masking the failure. * fix(sandbox): make E2B skill projection mounts best-effort _skill_projection_mounts called ensure_skill_projections with no guard, unlike Local/AIO's _ensure_skills_projection. A raise propagated out of _apply_mounts before the configured-mounts loop ran, so a skills projection failure dropped the operator's own configured mounts too - only caught by create()'s outer warning, with nothing applied at all. Swallow here and return an empty mount list on failure, matching the Local/AIO pattern: still fail-closed for skills, but no longer widens the blast radius to unrelated configured mounts. Review feedback from PR #4178. * docs(skills): document projection trade-offs flagged in review - _update_tree_digest: note the metadata-only (not content) hashing trade-off and why runtime writes through this codebase are still covered regardless (rebuild-under-lock + rename always changes inode). - LocalSandboxProvider.acquire: note the acquire-time self-heal cost (cheap on a fresh manifest, ~400ms rebuild under lock on stale/drift). - skill_projection_mutation: drop the no-op except-Exception-then-raise; a raise from the mutation already propagates past the yield with the view left cleared, no explicit re-raise needed. - provisioner README: spell out that hostPath skills volumes require the gateway and K8s node to share DEER_FLOW_HOST_BASE_DIR (single-node or shared storage), and that the custom/legacy volumes' hostPath type Directory (not DirectoryOrCreate) makes a violation of that assumption a visible Pod-creation failure instead of a silent empty mount. Review feedback from PR #4178. * fix(skills): lazily repair user projections * fix(skills): close projection review gaps * fix(skills): refresh user projection enable state * fix(skills): close projection review follow-ups * fix(skills): preserve state across projection writes --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Minh Vu
|
904cee4a72
|
fix(sandbox): push read_file ranges into sandbox reads (#3824)
* fix read_file range validation and passthrough * chore: format test_aio_sandbox.py * test: cover e2b sandbox read_file line-range behavior * fix(boxlite): support ranged file reads * fix(sandbox): reject non-positive start lines --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Yufeng He
|
8eb3be59bd
|
fix(sandbox): unwrap Overwrite-wrapped state in ensure_sandbox_initialized (#4429)
* fix(sandbox): unwrap Overwrite-wrapped state in ensure_sandbox_initialized The same fork-restored wrapper that crashed after_agent also reaches the sandbox init path, where sandbox_state.get() on the Overwrite object raises AttributeError. Share the unwrap helper from #4381's follow-up module deerflow/sandbox/overwrite.py and apply it at both init sites. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> * fix(sandbox): note why discarding fork_restored at the reuse sites is safe * fix(sandbox): unify the Overwrite unwrap helper and pin the fall-through - middleware.py now imports unwrap_sandbox from overwrite.py instead of keeping a second local copy whose docstring had already drifted; the shared helper covers both crash forms (subscript TypeError and the .get()-form AttributeError) - test the acquire fall-through: when the fork-restored id is gone from the provider, a fresh sandbox is acquired and the stale wrapped state is replaced by the plain acquired dict - the reuse-path test now also asserts runtime.state["sandbox"] stays wrapped, pinning the don't-treat-as-owned contract after_agent relies on Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> * fix(sandbox): unwrap Overwrite state in the sibling sandbox readers Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> --------- Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Ryker_Feng
|
aacb99cfd2
|
feat(lark): sidecar credential broker for sandbox lark-cli (Pattern B) (#4501)
* feat(lark): sidecar credential broker for sandbox lark-cli (Pattern B) Removes the plaintext Lark credential mounts (appSecret + OAuth tokens) from the sandbox container. A long-running broker sidecar owns lark-cli and the per-user config/data dirs and serves the command surface over Pod loopback; the sandbox gets only a forwarding shim on PATH, so the raw credential files never exist in the sandbox filesystem. - lark_broker.py: stdlib-only loopback broker (argv passthrough with shell=False, server-injected credential env, bounded I/O) + shim script constant + install-shim mode. - docker/lark-cli-broker: init(install-shim) + serve image. - provisioner: LARK_CLI_BROKER_IMAGE + provision_lark_cli_broker → shim init container + lark-cli-broker sidecar (config/data mounted sidecar-only); credentials dropped from the sandbox container; /api/capabilities reports lark_cli_broker_image. Broker supersedes the Pattern A init-container binary when both are configured. - gateway: lark_cli_env_overlay(broker=True) omits config/data env; sandbox_lark_broker_active() TTL-cached mode resolver; broker added to sandbox_runtime_mode / readiness and the settings UI. Opt-in and off by default (empty LARK_CLI_BROKER_IMAGE ⇒ no change). Closes #4338 * fix(lark): address Pattern B broker review findings (#4501) Follow-up to the sidecar credential broker addressing the PR #4501 review: - shim: split the on-PATH lark-cli into a /bin/sh launcher + Python shim body so broker mode fails loudly (exit 127, actionable message) instead of ENOEXEC when the sandbox image ships no python3; interpreter pinnable via DEERFLOW_LARK_BROKER_PYTHON. Launcher bakes in the shim's absolute path since $0 is the bare command name when run off PATH. - broker: drop the dead cwd payload field (broker can't see the sandbox FS) and document the command-surface-only / no-file-IO limitation. - broker: return a structured 500 JSON on unexpected exec errors so the shim gets a meaningful message, not an opaque transport failure; set a handler socket timeout to bound slow/stuck connections. - broker: add an opt-in DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS denylist that refuses secret-dumping subcommands before spawning the binary, forwarded from the provisioner sidecar. - gateway: tighten the per-bash-call broker probe timeout (1.5s) and cache negatives longer (300s) so non-broker remote-provisioner users don't pay a latency hit; guard the mode cache with a lock; drop the dead _probe_provisioner_lark_cli_init_image wrapper. - docs: remove the broken design-doc link from the broker README. Adds tests for launcher python resolution, cwd omission, denylist enforcement, 500-on-error, hot-path probe timeout + negative caching, and provisioner denylist-env wiring. |
||
|
Aari
|
d455a1815e
|
fix(sandbox): allow grep to search a single file (#4512) | ||
|
Yufeng He
|
6e6c078595
|
fix(sandbox): unwrap Overwrite-wrapped sandbox state in after_agent (#4381)
* fix(sandbox): unwrap Overwrite-wrapped sandbox state in after_agent
Fork-restored checkpoints can deliver the sandbox channel still wrapped
in langgraph.types.Overwrite: the rollback restore applies replace-style
writes through a state-mutation graph in delta checkpoint mode, and
after_agent/aafter_agent then crash subscripting the wrapper
("TypeError: 'Overwrite' object is not subscriptable") on the next
sandbox tool run in the forked conversation. Unwrap before reading the
sandbox id, and pin both hooks against an Overwrite-wrapped state.
Refs #4380 (bug 1 of 2; the history-loss half is a separate display path)
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
* fix(sandbox): don't release fork-restored parent sandboxes
The Overwrite-wrapped value replays the parent thread's sandbox state, so releasing it from the forked run would evict the parent's warm sandbox. _unwrap_sandbox now reports the wrapped form, and both after_agent hooks skip the release for it while keeping the normal path unchanged.
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
---------
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
|
||
|
Ryker_Feng
|
7aa314b4c1
|
feat: add Lark CLI integration (#3971)
* feat: add lark cli integration * fix: polish lark integration actions * feat: support lark incremental permissions * fix: detect lark authorization completion * fix: harden lark integration install * feat: expand lark auth scopes and reuse host auth in sandbox Default lark auth to least-privilege (recommend=false, base sign-in only) and expose the full set of lark-cli --domain business domains as native --domain grants instead of a 4-domain read-only mapping. Resolve the skill pack from the latest larksuite/cli GitHub release at install time with content-hash integrity, and surface version/runtime drift in status. Share the per-user lark-cli config/data profile between the Gateway Settings auth flow and agent conversations by mounting the integration dirs into the AIO sandbox and injecting the matching env for lark-cli commands, with an allowlisted extra_mounts path in the provisioner/K8s backend and traversal guards on integration paths. * style: fix lint issues from ruff and prettier Sort imports in the provisioner PVC test and re-wrap two long i18n description strings to satisfy backend ruff and frontend prettier CI. * fix(lark): address managed integration review feedback * fix(frontend): stabilize integrations settings e2e * test(sandbox): isolate remote backend legacy visibility check * test: fix backend unit failures after merge * Harden Lark integration review fixes * Format Lark integration E2E test * fix(lark): harden sandbox credential exposure and status disclosure Address willem_bd's security review on PR #3971: - Mount the per-user lark-cli config dir (long-lived appSecret) read-only into the AIO sandbox; only the refreshable-token data dir stays writable. - Redact host filesystem paths (install_path, cli.path) from GET /lark/status and the config/auth complete responses for non-admin callers, fail-closed on any auth error. - Document the npm postinstall trade-off (--ignore-scripts is not viable because @larksuite/cli fetches its platform binary in postinstall). - Document the sandbox credential trust boundary in AGENTS.md and README, pointing at the sidecar-broker follow-up (#4338). --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
luo jiyin
|
3b77a7401b
|
fix(sandbox): enforce E2B replica capacity limits (#4391)
* fix(sandbox): enforce E2B replica capacity limits (in-process) Add SandboxCapacityExceededError with diagnostic fields. Add overflow_policy (wait/reject/burst), acquire_timeout, and burst_limit config options. Implement atomic capacity reservation with a four-slot model: reserved / active / warm / transitioning. Transitioning slots close the window where active-to-warm or warm-to-active transitions appear to have zero occupied slots, which would let concurrent acquires exceed the configured replica ceiling. Re-route release, reclaim, and evict through transitioning counters. Add shutdown guard: reject waiters, kill VMs created during shutdown. Add 14 tests: policy enforcement, release+acquire race, warm-reclaim race, shutdown-waiter interaction, shutdown-during-create, and concurrent different-thread capacity assertion. Related: #4339 * fix: harden e2b sandbox capacity lifecycle * fix: retain e2b capacity during uncertain eviction * fix: serialize e2b tombstone eviction * fix: retain capacity after uncertain e2b cleanup * fix: track e2b remote operations during shutdown * fix(sandbox): validate E2B capacity config * fix(sandbox): classify capacity errors * fix(sandbox): harden E2B capacity lifecycle * test(sandbox): cover E2B review findings * docs(changelog): note E2B capacity behavior * docs(readme): explain E2B overflow handling * docs(backend): record E2B lifecycle rules * docs(sandbox): clarify destructive E2B reset * fix(sandbox): close E2B capacity race gaps --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Andrew Chen
|
ae510cb2e8
|
fix(sandbox): make an empty old_str a no-op in str_replace on any file (#4256)
str_replace guards the replacement with `if old_str not in content`, which
cannot reject an empty old_str -- `"" in content` is always true. So an
empty old_str reached `str.replace("", new_str)`, which inserts new_str at
every character boundary, and the tool rewrote the file while still
returning "OK":
old_str='', new_str='# H\n' -> OK, file silently prepended
old_str='', new_str='X', replace_all -> OK, 'XdXeXfX XmXaXiXnX(X)X:X\nX...'
The empty-file branch above it already handles this case (`if not content:
if not old_str: return "OK"`), and the existing test states the intent
directly: "An empty old_str is a no-op edit and remains a benign OK". That
contract just never held once the file had content.
The tool is registered by default (config.example.yaml) and its schema
declares old_str as a plain string with no minLength, so a model can emit
"" legitimately; read-before-write only compares a hash and lets it past.
Check old_str first so the no-op holds whatever the file contains. The
empty-file case folds into the same not-found branch, which keeps its
message and behaviour.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
hataa
|
10890e10a8
|
feat(authz): propagate trusted authorization principal context (#4203) | ||
|
Aari
|
bcee5a9061
|
refactor(sandbox): give the host→virtual output-mask regex a single owner (#4108)
* refactor(sandbox): give the host→virtual output-mask regex a single owner Two call sites rewrite host paths back to their virtual form in text that reaches the model — LocalSandbox._reverse_output_patterns (bash output) and sandbox.tools._compiled_mask_patterns (glob/grep/ls results) — and each built the same `escape(base) + boundary + tail` rule from its own copy. That duplication has already produced two bugs: #4035 added the segment boundary to the reverse patterns and missed the masking patterns, and #4053 had to add the same boundary to the other copy. Extract the rule into sandbox/path_patterns.py so a third copy cannot silently disagree. The extraction is not a pure move: the two sites disagree on the base. tools.py derives bases from _path_variants (which yields Windows spellings) and matches them against output whose separators it does not control, so it relaxes the separators inside the base; LocalSandbox resolves its bases from the running platform and must not be widened. That difference is now an explicit `separator_agnostic` parameter rather than an accident of two implementations. The boundary and tail constants are private: build_output_mask_pattern is the only supported spelling, so a third site cannot import the pieces and hand-roll a variant. Behavior is unchanged at both sites — pinned by tests that reproduce each pre-extraction expression byte-for-byte. * test(sandbox): pin the base the helper must not normalize Review notes on #4108. The committed snapshot compares the helper against hand-copied literals of the pre-extraction expressions, so its red-ness rests on those literals, not on the length of _BASES -- both sides compute the same expression, and 5k fuzzed bases produce zero byte-differences. Mutating the helper one clause at a time (12 mutations over the boundary, the tail and the escape/replace) shows the seven committed bases catch 11: the miss is a helper that normalizes its input by rstripping a trailing separator. Only a trailing-slash base or a Windows drive root catches that, and Path.resolve() / str(Path(...)) strip trailing slashes, so neither call site can produce the former. C:\ survives resolve() with its separator intact, so that is the one base worth adding. Also point local_sandbox's comment at path_patterns, the owner, instead of citing _content_pattern as the class reference, and drop the rationale it now duplicates from the owner's docstring -- a second copy of the explanation drifts the same way the second copy of the regex did. The site-specific half stays. Comments and test data only; no behavior change. |
||
|
Daoyuan Li
|
08fd218b83
|
fix(sandbox): use os.sep in reverse-resolve containment check on Windows (#4058)
* fix(sandbox): use os.sep in reverse-resolve containment check on Windows Path.resolve() always renders with the native separator (backslash on Windows), but _reverse_resolve_path's containment check hardcoded a "/" suffix when testing whether a resolved path is nested under a mapping's local root. Only the exact-root case (no separator needed) ever matched; every nested path fell through to the "no mapping found" branch and returned the raw host path -- leaking the real username and full directory tree into list_dir/glob/grep results and bash output masking instead of the virtual /mnt/user-data/... path. _is_read_only_path already does the equivalent check correctly via os.sep, so this aligns _reverse_resolve_path with that pattern: the containment check now compares with os.sep, and the extracted relative portion is normalized to forward slashes before being spliced into the (always POSIX-style) container path. Also fixes a same-file cosmetic bug in list_dir's virtual sub-directory overlay: it compared a bare child name (e.g. "workspace") against a set of full container paths, so the already-listed guard never matched and a mount whose subdirectory the underlying scan already found (the common case for /mnt/user-data/workspace, uploads, outputs) was appended a second time. Continues the same separator-bug class already fixed in this file by #3869 (forward-direction command resolution) and #4035 (reverse regex-boundary matching); neither touched this containment check. * test(sandbox): add host-OS-independent regression test for the os.sep containment fix _reverse_resolve_path's os.sep containment check (and the paired lstrip(os.sep).replace(os.sep, "/") extraction) has no test that would fail if reverted: backend CI runs only on ubuntu-latest, where os.sep == "/" makes the pre-fix hardcoded "/" and the current os.sep form observationally identical, so a plain POSIX-path test can't discriminate between them. Add a test that forces the Windows code path independent of host OS by monkeypatching os.sep to "\" and stubbing both the module's Path name and the sandbox's cached _resolved_local_paths to return backslash-joined strings, mirroring what real WindowsPath.resolve() produces -- without touching the filesystem or requiring an actual Windows host. Verified this fails with the raw host path leaking through when the os.sep fix is reverted to the hardcoded "/" form, and passes with the fix in place. |
||
|
Aari
|
c82fba41d9
|
fix(sandbox): guard the command path-translation regex with a segment boundary (#4110)
replace_virtual_paths_in_command matches the virtual root with no
segment-boundary lookahead:
re.compile(rf"{re.escape(VIRTUAL_PATH_PREFIX)}(/[^\s\"';&|<>()]*)?")
The trailing group needs a "/" to consume anything, so when the character after
/mnt/user-data is "-", ".", "_", a digit or a letter, the group matches empty
and the bare root still matches. The substitution then rewrites it to the
thread's host user-data directory and the rest of the sibling name rides along,
so a command naming a prefix sibling of the mount root is pointed at a real host
directory outside the mount contract:
cat /mnt/user-data-backup/secret.txt -> cat <host>/user-data-backup/secret.txt
which reads the host file. This is the same defect as #4035 (reverse patterns)
and #4053 (masking patterns), mirrored into the virtual->host direction; it is
the last unguarded member of that family.
The boundary class mirrors LocalSandbox._content_pattern's rather than
_command_pattern's: a virtual root can legitimately be followed by ":"
(PATH-style concatenation) or ",", which the shell-oriented class rejects, so
narrowing to it would stop translating paths that translate today. "$" covers a
command ending exactly at the root.
|
||
|
明年我18
|
0542d3c5f3
|
fix(sandbox): allow bash after cwd setup failure (#4051) | ||
|
黄云龙
|
d2ab5bb819
|
fix(sandbox): str_replace on empty file only returns OK when old_str is empty (#4079)
str_replace returned "OK" whenever the target file was empty, silently reporting success even when the model asked to replace a non-empty string that could not possibly be present. Only short-circuit to "OK" when old_str is also empty; otherwise return the standard not-found error. |
||
|
Aari
|
97e2268d52
|
fix(sandbox): stop glob/grep/ls from surfacing disabled skills' files (#4096)
* fix(sandbox): stop glob/grep/ls from surfacing disabled skills' files The disabled-skill gate checks the path a tool is given, but ls, glob and grep all descend from it and return other paths, so a root above a disabled skill still serves its files. glob and grep never called the gate at all; ls called it only on its own argument and still leaked from a category root. Add the entry gate to glob/grep, and filter what all three return through the existing fail-closed _is_disabled_skill_path. The verdict is memoized per skill because ExtensionsConfig.from_file() is uncached, so a per-match check would turn a 100-match grep into 100 config reads. * fix(sandbox): normalize trailing slashes in the disabled-skill path check Review follow-ups on the disabled-skill gate: - _extract_skill_name_from_skills_path returned "" instead of None for a category directory carrying a trailing slash. LocalSandbox.list_dir appends "/" to directories, so `ls /mnt/skills` yields "/mnt/skills/public/", giving parts ["public", ""]. The empty name skipped the `skill_name is None` short-circuit and fell through to a config read, landing on the right outcome only because unknown skills default to enabled. Drop empty segments so a trailing-slash category root takes the existing category-root branch. - ls_tool resolved the runtime user id twice per call; hoist it into a local, matching glob_tool/grep_tool. - Cover the CUSTOM path: custom/legacy skills resolve their enabled state through the per-user _skill_states.json, a different store from the public skills' extensions_config.json, and no automated test exercised it. |
||
|
Aari
|
1df9abc924
|
fix(sandbox): guard the output-masking regex with a segment boundary (#4053)
* fix(sandbox): guard the output-masking regex with a segment boundary `_compiled_mask_patterns` builds the same class of host→virtual matcher as `LocalSandbox._reverse_output_patterns`, but without the segment-boundary lookahead that one carries. The trailing group needs a separator to consume anything, so when the character after a host base is `-`, `.`, `_`, a digit or a letter, the group matches empty and the regex still matches the bare base. `replace_match` then takes its `matched_path == base` branch and rewrites the sibling: with `/mnt/skills` mounted at `.../skills`, output naming a sibling `.../skills-extra/data.txt` is handed to the model as `/mnt/skills-extra/data.txt` — a container path forward resolution explicitly refuses to map back, so reading it raises FileNotFoundError. This is the sibling site of #4035, which fixed the identical bug in `local_sandbox.py`. That PR's scope argument enumerated the prefix matchers in that file and missed this one; `mask_local_paths_in_output` runs on every glob/grep match and on local bash output. The boundary class mirrors `_content_pattern`'s, not `_command_pattern`'s: this runs over arbitrary command output, where a base can legitimately be followed by `,`, `:` or `\`, all of which the shell-oriented class rejects. * test(sandbox): anchor the sibling boundary at the ACP source too The sibling-rejection cases only fed the skills source. `_compiled_mask_patterns` builds every source's matcher in one loop, so the ACP workspace carried the same defect: nothing maps its parent, and `/mnt/acp-workspace-backup/hello.py` is unresolvable in both directions. User-data is the exception and is now pinned as such: `_thread_virtual_to_actual_mappings` also maps the virtual root `/mnt/user-data` to the three dirs' common parent, so a sibling of `outputs` is still inside a mount and has a real virtual path — the output is byte-identical with and without the boundary. That test is green on main; it guards the boundary from being narrowed into one that stops translating a mapped path. Reverting only `boundary` turns the 5 skills + 4 ACP cases red and leaves the 3 user-data cases green. |
||
|
黄云龙
|
08fdf61516
|
fix(sandbox): handle one-sided line ranges in read_file (#4078)
* fix(sandbox): honor single-bound line ranges in read_file read_file only sliced content when BOTH start_line and end_line were provided, so read_file(path, start_line=100) or read_file(path, end_line=50) silently returned the whole file. Handle one-sided ranges: default the missing bound (start->1, end->EOF), clamp start to 1, and return clear messages when start_line exceeds the file length or start_line > end_line. * address review: guard one-sided end_line <= 0 in read_file Add symmetric end_line guard and run the inverted-range check regardless of which bounds are explicit, so a lone end_line<=0 returns a clean error instead of a negative-index slice. Per @willem-bd review on #4078. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Aari
|
446ae98649
|
fix(sandbox): guard the reverse path-translation regex with a segment boundary (#4035)
* fix(sandbox): guard the reverse path-translation regex with a segment boundary
_reverse_output_patterns matches a mount's resolved local root with no
segment-boundary lookahead, unlike every other prefix match in this file: both
forward regexes carry one, and the three string-prefix resolvers compare against
`root + "/"`. Its optional trailing group needs a `/` or `\` to consume more, so
against a sibling that merely shares the prefix (".../skills-extra/data.txt") the
group matches empty and the regex still yields the bare root.
That extracted text then *equals* the mount root, which satisfies
_reverse_resolve_path's own `+ "/"` guard -- so the sibling is rewritten to
"/mnt/skills-extra/data.txt". Forward resolution refuses to map that back, so the
model is handed an absolute-looking container path it can never read. This runs
over every bash stdout/stderr and over read_file content.
Mirror _content_pattern's boundary class rather than _command_pattern's: this
regex sees arbitrary command output, where a root can legitimately be followed by
"," ":" or "\" -- all of which the shell-oriented class would reject, silently
dropping translations that work today. The trailing group keeps [/\\] so Windows
paths still match.
* test(sandbox): pin the end-of-output boundary that stops a host-path leak
The lookahead's `$` alternative had no test: deleting it left all 6866
backend tests green. It is not cosmetic. Output ending exactly at a mount
root -- printf '%s' "$PWD", a stripped last line, a capture truncated at
the buffer limit -- matches neither `/` nor `[^\w./-]`, so the pattern
fails and _reverse_resolve_paths_in_output emits the raw host path to the
model. main's unbounded pattern has no end-of-string problem; the
narrowing added here is what introduced the exposure.
Pin it: three prefixes, asserting both the container path and that the
host root is absent from the output. Deleting only `$` turns exactly
those three red.
Lift the boundary class and trailing group into named locals. The
compiled pattern is byte-identical; correctness hinges entirely on the
boundary class, so it should not sit at column 120 of a comprehension.
|
||
|
Aari
|
04a85b307a
|
fix(sandbox): scrub abbreviated *_PASS vars and Postgres PGPASSFILE from skill env (#4026)
* fix(sandbox): scrub abbreviated *_PASS vars and Postgres PGPASSFILE from skill env env_policy blocks the full PASSWORD/PASSWD spellings but not the ubiquitous abbreviated _PASS form (DB_PASS, SMTP_PASS, MYSQL_PASS, ...), whose value is the plaintext password itself, so every skill subprocess inherited it. Postgres's file-based credential sources PGPASSFILE (.pgpass locator) and PGSERVICEFILE (pg_service.conf, may carry a password) were likewise missed -- the psql analog of the MYSQL_PWD/REDISCLI_AUTH no-flag sources #4018 added. Collapse *PASSWORD*/*PASSWD* into a single *PASS* pattern (which subsumes both, covers the abbreviated family, and catches PGPASSFILE) and add PGSERVICEFILE to the exact-name denylist. *PASS* deliberately does not touch PWD/OLDPWD, which carry no PASS substring. Capability-handle vars (KUBECONFIG, DOCKER_CONFIG, GIT_ASKPASS, ...) are a broader, non-value-bearing class and are left out of this fix. test_secret_like_names_are_blocked already pins the contract; the new names are added to its parametrize list (red before, green after) and the benign-names test guards against over-scrubbing PWD/OLDPWD. * test(sandbox): pin the *_ASKPASS over-scrub that *PASS* introduces *PASS* also matches the credential-helper vars GIT_ASKPASS, SSH_ASKPASS and SUDO_ASKPASS, which main inherits. Each names a program whose purpose is to hand the caller a credential, so inheriting the pointer is the same leak class as inheriting the value -- a skill can simply invoke it. Scrubbing them is intended; pin all three in test_secret_like_names_are_blocked (red on main) so the behaviour is a decision rather than a side effect of the pattern's shape. test_benign_names_are_allowed could not have caught this: none of its 14 names carries a PASS substring, so it passes identically before and after the broadening and asserts nothing about it. Record that in its docstring -- the absence of a benign PASS-bearing name is a decision, and PWD/OLDPWD are the boundary the list does pin. Also note the incidental COMPASS_*/BYPASS_* over-scrub in the module comment (over-scrubbing is this module's fail-safe direction; required-secrets is the escape hatch), and list PGPASSFILE alongside PGSERVICEFILE in AGENTS.md. |
||
|
Aari
|
c11713c539
|
fix(sandbox): scrub MYSQL_PWD and REDISCLI_AUTH from the inherited skill env (#4018)
* fix(sandbox): scrub MYSQL_PWD and REDISCLI_AUTH from the inherited skill env env_policy blocks the MySQL and Redis connection strings (MYSQL_URL, REDIS_URL) because they embed a password, but not the password variables the same clients read directly: mysql/libmysqlclient takes MYSQL_PWD and redis-cli takes REDISCLI_AUTH with no further configuration. Both names carry no KEY/SECRET/TOKEN/PASSWORD/PASSWD substring, so they were inherited by every skill subprocess -- the leak surface this module exists to close (#3861). They need exact entries rather than a pattern: *PWD* would also strip PWD and OLDPWD. REDIS_AUTH is included as the common non-canonical variant. * Apply suggestions from code review - Distinguish REDIS_AUTH (defensive, non-canonical) from MYSQL_PWD and REDISCLI_AUTH (documented no-flag credential sources) in the comment. - Pin that request-scoped injection still overrides the host value for the newly blocked names, keeping required-secrets a working escape hatch. |
||
|
Vanzeren
|
e3137b13cf
|
fix(provisioner): gate legacy skills mount by user visibility (#3985)
* fix(provisioner): gate legacy skills mount by user visibility * fix(test): aio sandbox provider * fix: use shared legacy skill visibility helper for sandbox mounts |
||
|
Zheng Feng
|
dcb2e687d5
|
feat(channels): add GitHub as a webhook-driven channel (#3754)
* feat(channels): add GitHub event-driven agents (#3754) Add a webhook-driven GitHub channel with fail-closed webhook routing, deterministic per-agent PR/issue threads, mention-gated trigger fan-out, GitHub App token injection for sandboxed gh/git commands, and backend/AGENTS.md documentation. * fix(llm-middleware): classify bare IndexError as transient Upstream chat providers occasionally return 200 OK with an empty generations list (observed against Volces "coding" on ark.cn-beijing.volces.com). When that happens, langchain_core.language_models.chat_models.ainvoke raises ``IndexError: list index out of range`` at ``llm_result.generations[0][0].message`` and kills the run. Treat a bare IndexError reaching the middleware as a transient upstream-payload glitch and route it through the existing retry/backoff path instead of failing the whole agent run. The retry budget and backoff schedule are unchanged. Adds three regression tests covering the classifier and both the recover-on-retry and exhausted-retries paths. * fix(runtime): ignore stale LLM fallback markers from prior runs When a run on a thread ends with the LLM-error-handling middleware emitting a `deerflow_error_fallback`-marked AIMessage (e.g. after the IndexError empty-generations classification fix lands), that message is persisted to the thread's checkpoint as part of the messages channel. LangGraph replays the full message history in `stream_mode="values"` chunks, so every subsequent run on the same thread re-streams the stale fallback marker — and the worker's chunk scanner faithfully picks it up, flipping `RunStatus.success` to `RunStatus.error` for runs that themselves had no LLM failure at all. Snapshot the set of pre-existing message ids from the pre-run checkpoint and thread it through `_extract_llm_error_fallback_message` / `_try_extract_from_message` as a filter. Markers on history messages are ignored; markers on fresh messages produced during this run still trip the error path. Falls back to an empty set when the checkpointer is absent or the snapshot can't be captured, preserving the prior behavior on first-run / no-state paths. Adds unit tests for the new filter (helper-level and `_collect_pre_existing_message_ids`) plus an integration test exercising the full `run_agent` path with a stale history checkpointer. * fix(channels): make github channel fire-and-forget to avoid httpx.ReadTimeout on long runs GitHub agent runs (clone -> edit -> test -> push -> PR) routinely exceed the langgraph_sdk default 300s read deadline. The manager's runs.wait call kept an HTTP stream open for the entire run lifetime, so the long run blew up with httpx.ReadTimeout and the outer except branch then released the dedupe key and emitted a false 'internal error' outbound. The GitHub channel's outbound send is log-only by design: agents post to the issue/PR via the gh CLI in the sandbox when they choose to comment or create a PR. There is nothing for the manager to ferry back, so the long-poll was pure overhead. This change adds ChannelRunPolicy.fire_and_forget (default False) and sets it True for the github channel. When fire_and_forget is True, _handle_chat dispatches via client.runs.create (short POST, returns once the run is pending) instead of client.runs.wait, and skips the response-extraction + outbound-publish block. ConflictError on a busy thread still trips the standard THREAD_BUSY_MESSAGE path so behavior on the busy case is preserved for any future non-github fire-and-forget channel. Other (non-github) channels are unchanged: their policy defaults fire_and_forget=False and they continue to dispatch via runs.wait. Adds 6 regression tests in tests/test_channels.py::TestGithubFireAndForget: - Default ChannelRunPolicy.fire_and_forget is False. - The github policy registers fire_and_forget=True. - github inbound calls runs.create, not runs.wait, with the right kwargs. - github inbound publishes no outbound on success. - ConflictError from runs.create still emits THREAD_BUSY_MESSAGE. - Non-github channels (slack) still dispatch via runs.wait. * test(lead-agent): accept user_id kwarg in skill-policy test stubs The two GitHub-channel tests added in #3754 stubbed _load_enabled_skills_for_tool_policy with a lambda that only accepted `available_skills` and `app_config`, but the real function (and its call site in agent.py) also passes `user_id`. This raised TypeError on every run, failing backend-unit-tests. Add `user_id=None` to match the three sibling stubs in the same file. * refactor(gateway): disambiguate context-key set names The two frozensets _INTERNAL_ONLY_CONTEXT_KEYS and _CONTEXT_ONLY_KEYS shared a confusable "CONTEXT_ONLY" token in different orders, and the first broke the _CONTEXT_<X>_KEYS pattern of its sibling _CONTEXT_CONFIGURABLE_KEYS. Rename to make the distinct axes explicit: _CONTEXT_INTERNAL_CALLER_KEYS - WHO: internal callers (scheduler) only _CONTEXT_RUNTIME_ONLY_KEYS - WHERE: runtime context only, never configurable Pure rename, no behavior change. |
||
|
Xinmin Zeng
|
576577bd32
|
feat(channels): expose IM channel_user_id to sandbox commands as DEERFLOW_CHANNEL_USER_ID (#3926)
* feat(channels): expose channel_user_id to sandbox commands as DEERFLOW_CHANNEL_USER_ID IM-channel skills need the sender's platform identity (Feishu open_id, Slack Uxxx, ...). The channel manager already writes channel_user_id into body.context, but the Gateway whitelist dropped it. Forward it into the runtime context only (never configurable, which is checkpointed), and have bash_tool export it as a fixed env var through a shell-quoted command prefix. The identity deliberately does not ride execute_command(env=...): that channel is reserved for request-scoped secrets, and a non-empty env switches AioSandbox onto the bash.exec path (fresh session per call, image >= 1.9.3 required), which would have broken every IM bash command on older sandbox images and abandoned persistent-shell semantics on new ones. A command-string export keeps the legacy path, stays visible in audit logs (it is an identifier, not a secret), and gives per-call correctness in group chats where one thread and sandbox are shared by senders with different platform ids. Skipped on the Windows local sandbox, whose PowerShell/cmd.exe fallback has no POSIX export. Part of #3914 * feat(channels): propagate channel_user_id to subagents; cap value length Review findings from the pre-PR verification pass: - Subagent delegation dropped the sender identity: task_tool now captures channel_user_id from the parent runtime context and the executor forwards it into the subagent's context, mirroring the guardrail attribution fields (user_role/oauth_*/run_id). Without this, bash commands delegated via task lost the group-chat sender's id. - body.context is client-writable on web requests, so values over 256 chars are ignored instead of bloating every command string sent to the sandbox. * fix(channels): set-or-unset channel_user_id so identity is per-call regardless of AIO session persistence Review (willem-bd): the identity export could leak across senders in a shared group-chat AIO sandbox. The AIO no-env path reuses a persistent shell session (the class-lock reason, #1433), and the 256-char/type guard made some commands carry no prefix — so a dropped-id command could resolve the id a previous sender exported. Make per-call correctness independent of session semantics: an IM-channel command (channel_user_id present in context) now always carries an explicit prefix — export VAR=<quoted> for a valid id, or unset VAR for an unusable one (empty / non-str / over the cap). Non-IM runs (no key) are untouched. A prefix unset has none of the '& ; unset' suffix hazard raised earlier. Verified on a real AIO 1.11.0 container: the no-id shell path auto-creates a session per call (does not persist today), but an explicit shared session DOES persist (export stale-A -> readback [stale-A]); the unset prefix clears it (-> []). So the fix holds even on an image whose no-id path persists. Regression tests cover the dropped-id group-chat window and the non-IM passthrough. Part of #3914 * test(channels): align channel_user_id task test with new Command return shape The merge from main changed task_tool to return a Command(update=...) instead of a plain string; update the assertion to extract the tool message via the existing _task_tool_message helper, matching the sibling tests. Fixes the CI backend-unit-tests failure introduced by the merge. |
||
|
Zhipeng Zheng
|
53a80d3ad1
|
feat(skills): per-user custom skill isolation with sandbox mounting (#3889)
* feat(skills): per-user skill isolation (#2905) Implement user-scoped skill storage that isolates custom skills between users while sharing public skills globally. Key changes: - Add UserScopedSkillStorage class for per-user custom skill directories - Introduce get_or_new_user_skill_storage() factory with user_id context - Auth middleware sets effective_user_id for request-scoped storage - Agent/prompt/middleware now use user-scoped storage and prompt cache - Sandbox mounts user-scoped skill directories for search/read tools - Add validate_skill_file_path() to SkillStorage for path security - Migration script supports --all-users bulk migration - Frontend: add editable field to Skill type, error check in enableSkill - All skill categories can be toggled (custom skills default to enabled) - Update skill-creator SKILL.md with isolation-aware instructions Tests: - Add test_user_scoped_skill_storage.py (new) - Update all existing skill tests for user-scoped storage - Update sandbox, client, and router tests * fix(skills): address second-round PR review feedback (#3889) - P1-1: restrict legacy skill mount to users without custom skills - P1-2: fail-closed for _is_disabled_skill_path (OSError → return True) - P2-1: AND-merge global extensions_config skill disabled state - P2-2: atomic write for _skill_states.json (mkstemp + replace) - P2-3: normalize X-DeerFlow-Owner-User-Id in trusted boundary - P2-4: LRU-bounded _enabled_skills_by_config_cache (OrderedDict, maxsize=256) - P2-5: clear global prompt cache on PUBLIC skill toggle - P2-6: invalidate skill caches on client.update_skill * fix(tests): correct tool policy test after merge * fix(skills): use DEFAULT_SKILLS_CONTAINER_PATH in UserScopedSkillStorage The "/mnt/skills" literal in UserScopedSkillStorage.__init__ triggers test_skill_container_path_defaults::test_mnt_skills_literal_is_owned_by_skill_constants_module on CI. Migrate the default to the existing deerflow.constants constant, matching the pattern already used by LocalSkillStorage, SkillStorage, and the durable/tool_error middlewares. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
AochenShen99
|
66b9e7f212
|
feat: emit structured runtime metadata (follow-up#3887) (#3906)
* feat: emit structured runtime metadata * fix: avoid subagent import cycle in replay gateway * fix: preserve legacy subtask result parsing * refactor: tighten runtime metadata contracts * fix(middleware): keep recovery hint on task exception wrapper content The structured-metadata stamp overwrote the wrapper text with the bare task-failure message, dropping the model-facing 'Continue with available context, or choose an alternative tool.' guidance that every other tool exception keeps. Append the shared hint after the formatted message. * fix(subagents): require lowercase hex for result_sha256 reader Length-only validation accepted any 64-char string; a faulty serializer or relaying wrapper could store a non-digest value in the delegation ledger. Enforce the producer's hexdigest shape with a fullmatch. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Nan Gao
|
b81334ccfe
|
feat(middlewares): deterministic read-before-write version gate for file tools (#3911) (#3912)
* docs(specs): read-before-write gate design for issue #3857 output layer * docs(plans): read-before-write gate implementation plan (#3857) * refactor(sandbox): extract read_current_file_content helper (#3857) * feat(middlewares): read-before-write version gate for file tools (#3857) * test(middlewares): pin async read-before-write gate paths (#3857) * feat(config): wire ReadBeforeWriteMiddleware into runtime chain, default on (#3857) * docs(sandbox): document read-before-write gate in tool docstrings and AGENTS.md (#3857) * docs(plans): align plan doc with landed config_version (17) and drop machine-specific paths Addresses Copilot review comments on #3912. * fix(middlewares): read-before-write gate — error-string sandboxes fail open; serialize gate+execution per path (#3912 review) - AIO/E2B read_file reports failures (incl. missing files) as 'Error: ...' strings instead of raising; the gate treated that string as existing file content and blocked first-write creation. Error-string reads now count as uninspectable: gate fails open, no mark is stamped. - LangGraph runs one AIMessage's tool calls concurrently, so two same-turn writes could both pass on one stale mark before either mutation landed (and a read mark could hash a version the model never saw). Gate check + tool execution (and read + mark stamping) now share a per-(thread, path) critical section, separate from the tool-internal file_operation_lock. |
||
|
bjtolo
|
cd982d6751
|
fix(sandbox): normalize Windows backslash paths to forward slashes in bash commands (#3869)
* fix(sandbox): normalize Windows backslash paths to forward slashes in bash commands
On Windows, `replace_virtual_paths_in_command()` and
`LocalSandbox._resolve_paths_in_command()` resolve virtual paths
(/mnt/skills, /mnt/user-data, /mnt/acp-workspace, custom mounts) to
host paths with backslashes (C:\Users\admin\...). Bash interprets
\U, \a, \d, \s, \n, \t as escape sequences, mangling the path.
Fix: add `.replace("\\", "/")` to all path resolution callbacks so
resolved paths use forward slashes, which bash handles correctly on
all platforms and Windows APIs/Python's open() accept natively.
Fixes #3865
* test(sandbox): regression tests for Windows backslash path normalization
Covers replace_virtual_paths_in_command and LocalSandbox._resolve_paths_in_command
with Windows-style backslash paths, asserting no backslashes survive in output.
Closes #3865
* test(sandbox): fix ACP test failure, keep 6 passing regression tests
Removed test_acp_workspace_no_backslash which triggered path traversal
validation. Remaining 6 tests cover user-data (3), skills (1), and
LocalSandbox custom-mount (2) paths — all with Windows backslash paths.
* test(sandbox): restore ACP Windows path normalization coverage
|
||
|
Xinmin Zeng
|
09988caf95
|
feat(skills): request-scoped secrets for skills (closes #3861) (#3871)
* feat(sandbox): per-call env injection + platform-secret scrubbing for skills Add an env parameter to Sandbox.execute_command (abstract + local + AIO) so request-scoped secrets can be injected into skill subprocesses, and scrub platform credentials (*KEY*/*SECRET*/*TOKEN*/*PASSWORD*/*CREDENTIAL*) from the inherited environment by default so scoped injection is not security theatre. LocalSandbox always passes an explicit scrubbed env; AioSandbox routes env-bearing commands through bash.exec(env=) on a fresh session and leaves the legacy persistent-shell path unchanged. Part of #3861. BEHAVIOR CHANGE: execute_command no longer inherits the full os.environ; Windows encoding tests updated to assert the scrubbed dict. * feat(skills): parse required-secrets frontmatter declaration Add SecretRequirement and Skill.required_secrets, and parse the required-secrets SKILL.md frontmatter field (a string list or {name, optional} mappings), dropping malformed entries with a warning so one bad declaration does not invalidate the skill. The declared name is both the context.secrets key and the env var injected at activation. Part of #3861. * feat(runtime): request-scoped secret carrier (context.secrets) Add SECRETS_CONTEXT_KEY + extract_request_secrets, centralising the context.secrets carrier contract. The existing context passthrough (build_run_config -> _build_runtime_context) already carries the sub-key to runtime.context without mirroring it into configurable; characterization tests lock that behaviour. Part of #3861. * feat(skills): inject declared secrets at slash-activation into bash env Binding point A: when a skill is slash-activated, SkillActivationMiddleware resolves its declared required-secrets against the request's context.secrets and writes the per-run injection set to runtime.context. The bash tool forwards that set to execute_command(env=). A skill cannot harvest a host platform credential (is_host_platform_secret guard, cf. GHSA-rhgp-j443-p4rf), and injected values are redacted from bash output (mask_secret_values) so an echoed secret never re-enters the prompt/trace. Part of #3861. * test(skills): lock the five secret leak surfaces + add trace redaction helper Regression tests assert the secret value is absent from all five surfaces: prompt (activation message), checkpoint (graph state vs context separation), audit (journal records names only), trace (metadata builder never copies context; never mirrored to configurable), and stdout (mask_secret_values). Add redact_secret_context_keys as a defensive helper for any context serialization. Part of #3861. * docs(backend): document request-scoped secrets for skills Add Request-Scoped Secrets subsection (Skills) + env policy note (Sandbox) and the execute_command(env=) signature change, per the doc-sync policy. Part of #3861. * fix(skills): close gaps found by end-to-end verification of request-scoped secrets Real-gateway e2e + independent review of #3861 surfaced three defects, now fixed: 1. Slash activation never fired in the live chain. InputSanitizationMiddleware wraps user input in BEGIN/END markers before SkillActivationMiddleware sees it, and the original text was only preserved when an upload or IM channel set it. For a plain text message the slash command became undetectable, so no secret was ever resolved. Fix: the sanitizer now setdefaults the pre-wrap text into ORIGINAL_USER_CONTENT_KEY (additive; sanitization behaviour unchanged), so slash activation works for all messages. Pre-existing latent bug surfaced here. 2. The raw request config (with context.secrets) was persisted to runs.kwargs_json and echoed by the run API (RunResponse.kwargs). Fix: redact_config_secrets() strips secret-bearing context keys from the persisted/echoed copy in start_run; the live config that drives the run keeps them. build_run_config now also sets configurable.thread_id on the context path (the checkpointer requires it). 3. Connection-string credentials (DATABASE_URL, REDIS_URL, SENTRY_DSN, GH_PAT, ...) were not scrubbed from the inherited sandbox env. Fix: env_policy adds a *DSN* pattern plus an explicit connection-string denylist (no blanket *URL* — benign service URLs stay readable). Verified end-to-end via a real gateway run (real LLM + skill activation + bash): the secret reaches the sandbox subprocess and appears in NONE of prompt, trace, checkpoint, audit, stdout, runs.kwargs_json, or the run API. Part of #3861. * docs(backend): document the env scrub, persistence redaction, and sanitizer interaction Sync the Request-Scoped Secrets section with the verification-driven fixes: inherited-env scrub (incl. connection-string denylist), run-record/run-API redaction as the 6th sealed leak surface, and the sanitizer preserving original content so slash activation fires. Part of #3861. * fix(skills): inject caller secret over scrubbed host value; drop redundant host-name guard A real-world demo (a skill calling a third-party cloud API with a request-scoped key) exposed that the is_host_platform_secret guard was both wrong and harmful: it refused to inject a caller-supplied secret whenever a same-named variable existed in the Gateway env — which is exactly the #3861 use case (a per-user key overriding a shared platform key). The guard was also redundant: build_sandbox_env already scrubs secret-looking names from the inherited env before injection, so a skill can never read a host credential — it only ever receives the caller's value. Remove the guard; the injected (caller) value simply wins over the scrubbed host value. Verified end-to-end: the agent called the real cloud API successfully with the caller's key, the host's same-named key was scrubbed and never used, and the caller's key leaked to none of the surfaces. Part of #3861. * fix(skills): address review on request-scoped secrets (#3861) Review fixes from PR #3871: - E2BSandbox.execute_command now accepts env/timeout and routes them to commands.run(envs=, timeout=). The bash tool passes env= unconditionally, so the prior signature (command only) raised TypeError on every e2b bash call and broke e2b deployments entirely. env=None stays backward-compatible. - SkillActivationMiddleware clears the active-secret set before resolving each activation, so a later skill in the same run never inherits an earlier skill's injection set (the #3861 contract: a skill only receives what the caller supplied AND that skill declared). - AioSandbox env path uses a dedicated _DEFAULT_HARD_TIMEOUT — bash.exec exposes no idle/no-change timeout, so the prior reuse of the legacy idle constant conflated wall-clock vs idle semantics. The env path also retries on the ErrorObservation signature now, sharing the legacy persistent-shell recovery contract. - mask_secret_values skips values below a minimum length floor so a short declared secret (e.g. "42") cannot shred unrelated bytes (exit codes, timestamps, sizes) of tool output. The secret is still injected into the subprocess; only the output mask skips it. session_id reuse on the env path is intentionally NOT added: a shared session could let request-scoped secrets ride the session env into later commands, which the SDK does not contractually forbid. The fresh-session choice matches the LocalSandbox model (each call is a fresh subprocess); the trade-off (consecutive env-bearing calls do not share cwd/venv/exports) is documented on _execute_with_env. |
||
|
Minh Vu
|
8fa6ed2b54
|
fix(gateway): fix oversized upload replacements deleting existing files (#3822)
* fix upload rollback on size limit * fix upload staging cleanup and listings |
||
|
DanielWalnut
|
fe82552023
|
fix(sandbox): stop blocking bash commands (e.g. servers) from hanging the turn (#3864)
* fix(sandbox): stop blocking bash commands from hanging the turn Starting a server through the host bash tool (e.g. `python -m http.server`) could hang the whole turn for the full 600s timeout. `LocalSandbox.execute_command` used `subprocess.run(capture_output=True)`, whose captured pipes are inherited by any process the command spawns — so a backgrounded long-lived process (`server &`) keeps the read end open and blocks `communicate()` until the timeout fires, even though the foreground command already returned. Commands that read stdin blocked the same way, and on timeout only the direct child was killed, leaving orphaned process groups. Rework the POSIX path to capture stdout/stderr via temp files instead of pipes, take stdin from /dev/null, and run the command in its own session/process group: - Backgrounded long-lived processes (servers) now return immediately while the process keeps running. - A command reading stdin gets immediate EOF instead of blocking. - A genuinely blocking foreground command is bounded by a configurable wall-clock timeout; on timeout the whole process group is killed and the agent gets an explanatory notice telling it to background long-lived processes. The timeout is configurable via `sandbox.bash_command_timeout` (default 600). The Windows path is unchanged. Adds focused regression tests and updates docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(sandbox): instruct the agent to background long-lived processes The code fix bounds a foreground server with a timeout, but the turn still waits the full timeout before the run continues. Add the prompt-side half: the bash tool description now tells the model to ALWAYS start long-lived processes (e.g. web servers) in the background with output redirected, so the tool returns immediately. The timeout notice points at the same readable workspace log path. Pins the guidance with a test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(sandbox): make fallback-kill exception explicit and observable Address automated review: the inner `except OSError: pass` in _terminate_process_group silently swallowed the case where the direct-child fallback kill found the process already gone. Make the intent explicit with a comment and a debug log instead of a bare pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(sandbox): address bash timeout review feedback * fix(sandbox): document fd cleanup races --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Czile
|
30841c3b9f
|
fix(sandbox): synchronize sandbox provider singleton lifecycle (+ concurrency regression tests) (#3730)
* fix(sandbox): synchronize sandbox provider singleton lifecycle get_sandbox_provider() used an unsynchronized check-then-create, so two OS threads (e.g. the main event loop and the Feishu channel thread, which runs its own loop) could double-initialize the provider. With AioSandboxProvider the overwritten instance leaks its idle-checker thread, since the only code that joins it (shutdown()) is reachable only through the reference that was overwritten. reset_sandbox_provider(), shutdown_sandbox_provider() and set_sandbox_provider() also touched the global without a lock, so a reset/shutdown racing an in-flight create could clear it mid-construction or tear down an instance another thread was about to return. Guard all four lifecycle sites with a single module-level threading.Lock and use double-checked locking in the getter, mirroring get_memory_storage(). * test(sandbox): add concurrent regression tests for provider singleton - 8 threads racing on cold start, synchronized with a threading.Barrier so the check-then-create race fires deterministically; asserts exactly one provider instance is created. - reset racing concurrent gets: asserts every returned value is a fully constructed provider (never None / half-built). * fix(sandbox): lock get read path, run provider callbacks outside the lock Addresses the three review findings on #3730: 1. get_sandbox_provider()'s read+return ran outside _provider_lock, so a concurrent reset/shutdown/set could null or tear the global between the check and the return, handing callers None / a torn instance. The hot read and the install reconciliation now both happen under the lock. 2. The non-reentrant _provider_lock was held across plugin-supplied callbacks (resolve_class import + provider __init__ in get; provider.reset()/shutdown() in reset/shutdown). A custom provider that re-entered these lifecycle functions would self-deadlock, and a slow teardown blocked every concurrent get(). resolve_class + construction now run outside the lock; reset/shutdown detach the reference under the lock and invoke the callback outside it. Tradeoff: racing cold-start callers may each construct a candidate. Exactly one is installed and returned to everyone; the losers (e.g. an AioSandbox instance that already started an idle-checker thread) are shut down so they do not leak the orphan thread #3721 is about. set_sandbox_provider() documents that it replaces but does not shut down the prior instance. 3. test_reset_racing_get reset the singleton to None *before* the barrier, so the racing reset was a no-op and never exercised reset-of-a-live-provider. It now populates the singleton up front so the reset tears down a live instance while getters read it. Tests: rename the cold-start test to assert "one installed singleton, observed by all" (construction is no longer single under the new design); add shutdown-vs-get, set-vs-get, and a losing-racer-shuts-down-its-orphan case. All five pass; the existing sandbox/middleware/mounts/uploads suites are green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Eilen Shin
|
67dd75db62
|
chore: remove unused private helper functions (dead code) (#3749)
These three module-private helpers have no callers anywhere in the repo (verified by a repo-wide identifier scan + direct greps; each appears only at its own definition): - agents/memory/updater.py: _create_empty_memory (a no-caller wrapper around storage.create_empty_memory, which is used directly elsewhere) - runtime/runs/worker.py: _extract_human_message (also drops the now-unused HumanMessage TYPE_CHECKING import and TYPE_CHECKING from typing) - sandbox/tools.py: _looks_like_unsafe_cwd_target (cwd safety is enforced via _is_allowed_local_bash_absolute_path) Underscore-prefixed = module-internal, so zero references is conclusive. Route handlers and @compiles-registered functions were excluded as false positives. Public-surface candidates are noted in the issue, not touched here. Fixes #3748. |