Skip to content

Commit b37df71

Browse files
fix: address Copilot review round 2 (PR #168)
Five new comments from Copilot's second review: README.md: - Spelled out env var names (AGENTIC_WORKSPACE_PLUGINS / _AGENTS) instead of the abbreviated /_PLUGINS / _AGENTS form that could cause copy-paste misconfiguration. docs/workspace.md: - inject() example now targets /workspace/CLAUDE.md (parent guaranteed by the image) instead of /etc/agentic/workspace/... which only exists when the orchestrator bind-mounts it. Added a comment explaining why. providers/workspaces/claude-cli/scripts/entrypoint.sh: - Security fix: __inject_safe_filter rejects plugin/agent names containing '/' or '..'. Previously a value like AGENTIC_WORKSPACE_PLUGINS='../etc' could escape the intended /etc/agentic/workspace/plugins/ mount. lib/python/agentic_isolation/agentic_isolation/workspace_files.py: - inject() now explicitly rejects trailing slashes; docstring is accurate. Path('/foo/') normalizes to /foo internally, so the earlier basename check didn't actually catch this. - Renamed test_inject_rejects_empty_basename to test_inject_rejects_root_path since the trailing-slash check now catches '/' first. - New test_inject_rejects_trailing_slash. docs/superpowers/specs/2026-05-12-workspace-injection-contract-design.md: - Spec snippet was showing chmod 644 but impl uses 600 (the change we made for round 1). Synced spec → 600 to remove drift. Tests: - 177 Python (+1 for trailing-slash test) - 7 integration green - ruff check + format clean - Image rebuilt and integration tests passed against fresh image.
1 parent 6f20087 commit b37df71

6 files changed

Lines changed: 46 additions & 8 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ just list-providers
254254

255255
`agentic-primitives` ships the workspace image — the controlled boundary every AI agent runs inside. The workspace has three responsibilities:
256256

257-
1. **Inject** orchestrator-supplied context (`CLAUDE.md`, plugins, subagents) via a bind-mount at `/etc/agentic/workspace/` + three optional env vars (`AGENTIC_WORKSPACE_CONTEXT` / `_PLUGINS` / `_AGENTS`).
257+
1. **Inject** orchestrator-supplied context (`CLAUDE.md`, plugins, subagents) via a bind-mount at `/etc/agentic/workspace/` + three optional env vars (`AGENTIC_WORKSPACE_CONTEXT`, `AGENTIC_WORKSPACE_PLUGINS`, `AGENTIC_WORKSPACE_AGENTS`).
258258
2. **Isolate** the agent's effects (tmpfs home, read-only context mount, network whitelisting, per-task volumes).
259259
3. **Observe** what the agent did (git hooks → JSONL on stderr, `--output-format stream-json` on stdout, output artifacts in `/workspace/artifacts/output/`).
260260

docs/superpowers/specs/2026-05-12-workspace-injection-contract-design.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ if [ -d "${INJECT_MOUNT}" ]; then
150150
ctx_src="${INJECT_MOUNT}/${AGENTIC_WORKSPACE_CONTEXT:-${INJECT_DEFAULT_CONTEXT}}"
151151
if [ -f "${ctx_src}" ]; then
152152
cp "${ctx_src}" "${INJECT_TARGET_CONTEXT}"
153-
chmod 644 "${INJECT_TARGET_CONTEXT}"
153+
chmod 600 "${INJECT_TARGET_CONTEXT}"
154154
fi
155155

156156
# 2. Per-workspace plugins (appended to existing AGENTIC_PLUGIN_FLAGS

docs/workspace.md

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,15 @@ mount = wf.bind_mount(workspace_dir, "/etc/agentic/workspace", read_only=True)
115115
container = client.containers.create(image, mounts=[mount], ...)
116116

117117
# Inject mode (generated content)
118+
# Note: inject() requires the parent dir to already exist inside the
119+
# container. /workspace and /tmp are guaranteed by the image; arbitrary
120+
# paths under /etc/agentic/workspace/ are NOT — that path only exists
121+
# when an orchestrator also bind-mounts it. For generated context, the
122+
# common pattern is to inject into /workspace/ (which the image creates
123+
# at build time) and let the entrypoint's section 5.5 leave that copy
124+
# untouched (since /etc/agentic/workspace/ isn't mounted in this flow).
118125
container = client.containers.create(image, ...)
119-
wf.inject(container.id, "/etc/agentic/workspace/CLAUDE.md", composed_bytes)
126+
wf.inject(container.id, "/workspace/CLAUDE.md", composed_bytes)
120127
container.start()
121128
```
122129

lib/python/agentic_isolation/agentic_isolation/workspace_files.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,12 @@ def inject(
7575
directories.
7676
7777
Raises ``ValueError`` if ``container_path`` is not an absolute
78-
path or has an empty basename (e.g. ``/`` or trailing slash).
78+
path, has an empty basename (e.g. ``/``), or has a trailing
79+
slash (which would imply "this is a directory" and is not a
80+
valid target for a single-file tar).
7981
"""
82+
if container_path.endswith("/"):
83+
raise ValueError(f"container_path must not end with '/', got {container_path!r}")
8084
target = Path(container_path)
8185
if not target.is_absolute():
8286
raise ValueError(f"container_path must be absolute, got {container_path!r}")

lib/python/agentic_isolation/tests/test_workspace_files.py

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,14 +85,28 @@ def test_inject_rejects_relative_path():
8585
raise AssertionError("expected ValueError")
8686

8787

88-
def test_inject_rejects_empty_basename():
89-
"""inject() raises ValueError on a path whose .name is empty (e.g. /)."""
88+
def test_inject_rejects_root_path():
89+
"""inject() raises ValueError on `/` (caught by the trailing-slash
90+
check, which also covers the empty-basename case)."""
9091
from agentic_isolation.workspace_files import WorkspaceFiles
9192

9293
wf = WorkspaceFiles(client=MagicMock())
9394
try:
9495
wf.inject("ctr", "/", b"x")
96+
except ValueError:
97+
pass
98+
else:
99+
raise AssertionError("expected ValueError")
100+
101+
102+
def test_inject_rejects_trailing_slash():
103+
"""inject() raises ValueError on a path ending with '/'."""
104+
from agentic_isolation.workspace_files import WorkspaceFiles
105+
106+
wf = WorkspaceFiles(client=MagicMock())
107+
try:
108+
wf.inject("ctr", "/workspace/", b"x")
95109
except ValueError as e:
96-
assert "basename" in str(e)
110+
assert "slash" in str(e) or "/" in str(e)
97111
else:
98112
raise AssertionError("expected ValueError")

providers/workspaces/claude-cli/scripts/entrypoint.sh

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,10 +214,23 @@ readonly INJECT_DEFAULT_CONTEXT="CLAUDE.md"
214214
readonly INJECT_PLUGIN_MANIFEST=".claude-plugin/plugin.json"
215215

216216
# --- Helpers --------------------------------------------------------------
217+
# Reject any name containing '/' or '..' so plugin/agent names supplied
218+
# via env can't escape the intended mount via path traversal. Caller
219+
# pipes a stream of names through this filter.
220+
__inject_safe_filter() {
221+
while IFS= read -r name; do
222+
[ -n "${name}" ] || continue
223+
case "${name}" in
224+
*/*|*..*|"") continue ;;
225+
esac
226+
printf '%s\n' "${name}"
227+
done
228+
}
229+
217230
__inject_names() {
218231
local explicit="$1" dir="$2" strip_ext="${3:-}"
219232
if [ -n "${explicit}" ]; then
220-
printf '%s\n' "${explicit}" | tr ':' '\n'
233+
printf '%s\n' "${explicit}" | tr ':' '\n' | __inject_safe_filter
221234
return
222235
fi
223236
[ -d "${dir}" ] || return

0 commit comments

Comments
 (0)