security: isolate host agents and sanitize subprocesses

[eshulman2]

Summary

• isolate host agents in a dedicated virtual, read-only filesystem with no shell backend
• enforce safe built-in tools and exact default-deny MCP tool allowlisting
• stage validated skills into the agent root and reject traversal and symlinks
• prevent implementation and reviewer shells from ordinarily inheriting container secrets
• pin Deep Agents 0.6.12 and document production-safe Podman, service-container, and future Kubernetes deployment controls

Security boundary

This phase intentionally leaves the top-level container environment, provider authentication, Google ADC mounts, Langfuse callbacks, tracing metadata, session correlation, and dynamic-skill behavior unchanged. inherit_env=False is defense in depth: sufficiently capable code may still inspect container-level secrets through mechanisms such as /proc.

Validation

• uv run ruff check src/forge/integrations/agents src/forge/skills/resolver.py src/forge/main.py src/forge/orchestrator/worker.py src/forge/config.py src/forge/cli.py containers/entrypoint.py tests/unit/integrations/agents
• targeted agent, skill, container entrypoint, sandbox backend, worker, and configuration unit suites
• git diff --check origin/main...HEAD

Deployment docs

See docs/security/agent-isolation.md for configuration, rootless Podman controls, hardened service-container settings, Kubernetes prerequisites, negative tests, canary rollout, incident response, and deferred gateway/proxy work.

[eshulman2]

Code review

Found 1 issue:

1. validate_agent_root() calls root.mkdir(parents=True, exist_ok=True, mode=0o700), but mkdir ignores mode when the directory already exists (and the process umask can weaken it on creation). If the agent root pre-exists with permissive permissions (e.g. 0o755/0o777), validation still passes and the isolation boundary this PR establishes is silently weakened — other local users/processes could read the agent workspace and skill tree or plant skills. There is no follow-up chmod. Consider an explicit root.chmod(0o700) after mkdir. (bug — undermines the PR's own isolation goal)

forge/src/forge/integrations/agents/security.py

Lines 49 to 55 in 5faf4ed

	def validate_agent_root(root: Path, project_root: Path, workspace_base: str = "") -> Path:
	"""Create and validate a dedicated agent root that cannot expose Forge data."""
	if root.is_symlink():
	raise ValueError(f"Agent root must not be a symlink: {root}")
	root.mkdir(parents=True, exist_ok=True, mode=0o700)
	resolved = root.resolve(strict=True)
	project = project_root.resolve(strict=True)

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[eshulman2]

Addressed the existing review feedback in f71b3d2:

• enforce 0700 on an existing or newly created agent root, after overlap validation so rejected paths are never chmodded
• add regression coverage for permission enforcement and rejected-path behavior

Independent review also found and fixed an MCP/built-in name collision: an allowlisted MCP tool named execute, write_file, or edit_file could cause the additive Deep Agents built-in with the same name to pass the name-based middleware. Those identifiers now fail closed, with regression coverage and documentation.

Validation: targeted Ruff checks pass; all 2,149 unit tests pass (one pre-existing AsyncMock warning). No additional blocking findings in the PR diff.

[eshulman2]

Automated inline review. Overall this is a solid hardening pass (isolated host-agent root, exact tool/MCP allowlisting, sanitized subprocess env, symlink/traversal guards) and CI is green. One finding below is verified against the actual langchain/langgraph tool-merge code and should be fixed before merge since it undermines this PR's own stated goal (exact allowlisting instead of trusting names). The other two are lower-severity notes, not blockers.

[eshulman2]

Re-reviewed after the latest push. Commit ad58dec correctly fixes both prior findings: the MCP/safe-builtin-tool collision check (agent.py:404, now checks SAFE_BUILTIN_TOOLS | PROHIBITED_BUILTIN_TOOLS, with a new test) and the stale-skill pruning (security.py, committed skills now rebuilt exactly into a separate committed-skills dir each startup, with tests). Good fixes.

However, this same commit introduces a new regression by only updating the host-agent skill path and missing the container-agent one — see inline comment. Also note CI's Lint job is currently red on this commit (ruff format check on agent.py), which is a mechanical fix, not a design issue.