-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: bind-mount local --target by default to avoid slow file-by-file … #840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
c7c7096
7c1e37c
6e62ea7
b04bfa9
e190683
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1225,11 +1225,16 @@ def collect_local_sources(targets_info: list[dict[str, Any]]) -> list[dict[str, | |
| workspace_subdir = details.get("workspace_subdir") | ||
|
|
||
| if target_info["type"] == "local_code" and "target_path" in details: | ||
| # Local directory targets bind-mount read-only by default (issue | ||
| # #725): the SDK LocalDir entry copies a tree into the sandbox | ||
| # file-by-file, which stalls for hours on large repos. A bind mount | ||
| # is applied at container-create time and is effectively instant. | ||
| # An explicit ``mount: False`` still forces the file-by-file copy. | ||
| local_sources.append( | ||
| { | ||
| "source_path": details["target_path"], | ||
| "workspace_subdir": workspace_subdir, | ||
| "mount": bool(details.get("mount", False)), | ||
| "mount": bool(details.get("mount", True)), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Docker uses a remote daemon, this default passes the CLI machine's local path directly as a host bind source. The daemon resolves that path on its own host, so an ordinary Prompt To Fix With AIThis is a comment left during a code review.
Path: strix/interface/utils.py
Line: 1237
Comment:
**Remote Daemon Cannot See Target**
When Docker uses a remote daemon, this default passes the CLI machine's local path directly as a host bind source. The daemon resolves that path on its own host, so an ordinary `--target` can fail during container creation or expose the wrong directory; the previous `LocalDir` path uploaded the client-side files.
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| """Regression tests for issue #725: local --target hangs on file-by-file import. | ||
|
|
||
| Root cause: a local directory ``--target`` was handed to the SDK ``LocalDir`` | ||
| manifest entry, which copies the tree into the sandbox file-by-file at | ||
| ``session.start()`` — hours-long on large repos. The fix bind-mounts local | ||
| targets read-only by default (fast, applied at container-create time). | ||
|
|
||
| The bug-condition exploration test asserts the *negation* of the bug condition | ||
| C(X): a local_code target must resolve to a bind mount, not a copied LocalDir | ||
| entry. It fails on the unfixed code (confirming the bug) and passes after the | ||
| fix. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from agents.sandbox.entries import LocalDir | ||
|
|
||
| from strix.interface.utils import collect_local_sources | ||
| from strix.runtime.session_manager import build_session_entries | ||
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| from pathlib import Path | ||
|
|
||
|
|
||
| def _local_target(target_path: str, workspace_subdir: str = "repo") -> dict[str, Any]: | ||
| """A plain ``--target <dir>`` target_info (no explicit --mount).""" | ||
| return { | ||
| "type": "local_code", | ||
| "details": {"target_path": target_path, "workspace_subdir": workspace_subdir}, | ||
| "original": target_path, | ||
| } | ||
|
|
||
|
|
||
| def test_bug_condition_local_target_is_bind_mounted_not_copied(tmp_path: Path) -> None: | ||
| """BUG-CONDITION EXPLORATION (issue #725). | ||
|
|
||
| A local directory target must be configured as a bind mount, NOT a | ||
| file-by-file LocalDir copy. Expected to FAIL on unfixed code, where a plain | ||
| --target defaults to a LocalDir copy. | ||
| """ | ||
| (tmp_path / "app.py").write_text("print('hi')\n", encoding="utf-8") | ||
|
|
||
| sources = collect_local_sources([_local_target(str(tmp_path))]) | ||
| entries, bind_mounts, _staged = build_session_entries(sources) | ||
|
|
||
| # Negation of C(X): bind-mounted, and NOT a copied LocalDir entry. | ||
| assert entries == {}, "local --target must not be a file-by-file LocalDir copy" | ||
| assert [m["target"] for m in bind_mounts] == ["/workspace/repo"] | ||
| assert all(not isinstance(v, LocalDir) for v in entries.values()) | ||
|
|
||
|
|
||
| def test_local_target_bind_mount_is_read_only(tmp_path: Path) -> None: | ||
| sources = collect_local_sources([_local_target(str(tmp_path))]) | ||
| _entries, bind_mounts, _staged = build_session_entries(sources) | ||
|
|
||
| assert bind_mounts == [ | ||
| { | ||
| "source": str(tmp_path.resolve()), | ||
| "target": "/workspace/repo", | ||
| "read_only": True, | ||
| } | ||
| ] | ||
|
|
||
|
|
||
| def test_explicit_copy_still_supported(tmp_path: Path) -> None: | ||
| """An explicit mount=False local source is still copied (escape hatch preserved).""" | ||
| target = _local_target(str(tmp_path)) | ||
| target["details"]["mount"] = False | ||
|
|
||
| sources = collect_local_sources([target]) | ||
| entries, bind_mounts, _staged = build_session_entries(sources) | ||
|
|
||
| assert bind_mounts == [] | ||
| assert isinstance(entries["repo"], LocalDir) | ||
|
|
||
|
|
||
| def test_repository_source_is_unaffected(tmp_path: Path) -> None: | ||
| """FR2: cloned repositories retain their copy behavior (not bind-mounted).""" | ||
| repo = { | ||
| "type": "repository", | ||
| "details": {"cloned_repo_path": str(tmp_path), "workspace_subdir": "clone"}, | ||
| } | ||
| sources = collect_local_sources([repo]) | ||
| entries, bind_mounts, _staged = build_session_entries(sources) | ||
|
|
||
| assert bind_mounts == [] | ||
| assert isinstance(entries["clone"], LocalDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An oversized
local_codetarget withmount: Falsestill reaches this warning, butcollect_local_sourcespreserves that flag andbuild_session_entriescreates a file-by-fileLocalDircopy. The command now proceeds into the same long import that the size guard previously blocked while incorrectly reporting that the target will be bind-mounted.Prompt To Fix With AI