Repository navigation
Conversation
7f33621 to
71cf87b
Compare
0ce7525 to
c78629b
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real gap: current terminal_tool() and execute_code() do not invoke is_write_denied() while native file operations do (tools/file_operations.py:1386). The direction is useful, but the heuristic needs correction before salvage.
Problems
tools/code_execution_tool.py:1148omitsr+, even thoughr+is write-capable;open('/outside/existing', 'r+')passes this guard.tools/code_execution_tool.py:1122classifies everyPath.open()as a write, but its default mode is read-only.tools/terminal_tool.py:336-338resolves relative targets throughis_write_denied()before terminal execution resolves its cwd.is_write_denied()uses the parent process cwd (agent/file_safety.py:101), socd-based writes can be misclassified and the issue'scdbypass is not reliably addressed.
Suggested changes
- Make Python mode handling cover
+and preserve read-onlyPath.open(). - Resolve targets against the actual execution cwd and add endpoint-level regressions for
r+, defaultPath.open(), and relative targets aftercd. - Salvage against current main; GitHub marks the branch dirty and its base is 1,132 commits behind HEAD.
Automated hermes-sweeper review.
c78629b to
f49b4af
Compare
|
Addressed in f49b4af87, force-pushed on top of current main. This rework now covers the three points from #39004 (review):
Validation on this branch:
|
f49b4af to
88f293f
Compare
88f293f to
86fb330
Compare
Field evidence for the
|
|
too large to review safely This PR changes 1069 production lines before tests and docs. Please split it or add a focused justification if it should stay together. Signed: GPT-5.6-luna-high in Codex |
|
I reproduced #36645 on macOS across four distinct local child-spawn shapes: #39004 is the best existing direction I found: it establishes an explicit I have a tested macOS Seatbelt provider candidate against current main that wraps the Because #39004 already owns the issue and touches the generic policy shape, I do not |
|
@egilewski Keeping this together lets us review the complete opt-in execution boundary. Terminal, execute_code, file tools, and prompt probing all create or reuse environments, so they must share the same policy checks and environment identity. Docker mount validation enforces the host-write restriction, and cleanup tracks those same identities. The review order is execution_policy.py, Docker enforcement, then the callers and cleanup. The default remains legacy, workspace scope supports Docker, and other backends refuse before execution. Additional adapters can follow in separate PRs. |
|
Correction to my earlier macOS candidate note: adversarial testing found that the Seatbelt predicate I described is not a sufficient direct-write boundary. If a hard link already exists under an allowed root and points to the same inode as a path outside that root, writing through the allowed name changes the protected path too. My original test only proved that a confined child could not create a new hard link; it missed this pre-existing alias. I reproduced the failure on macOS and have rejected the candidate. Please do not treat my earlier test summary as evidence for a native-local adapter. The four-spawn-seam requirement still stands, but a replacement must eliminate this inode alias class or claim and enforce a materially narrower boundary. |
Summary
HERMES_WRITE_SAFE_ROOTremains the nativewrite_fileandpatchpolicy it has always been. This rebuild removes the bypassable command and Python parsing from this branch and addsterminal.execution_write_scope: workspacefor sessions that require an execution boundary.Docker is the first supported workspace adapter. It validates writable host mappings against an immutable session workspace, keeps credentials, skills, caches, and profile images read-only, and isolates private container storage. Local, SSH, and other adapters return a clear unsupported error before creating a process, connection, container, or PTY session.
Changes
tools/environments/execution_policy.py: shared immutable policy, bounded environment identity, Docker mapping validation, and typed capability failures.legacybehavior plus Docker-only workspace support and explicit non-publication semantics.Validation
HERMES_WRITE_SAFE_ROOTnative file writelegacydefaultTest plan
python -m pytest tests/tools/test_execution_write_policy.py tests/tools/test_terminal_task_cwd.py tests/tools/test_terminal_tool.py tests/tools/test_file_tools.py tests/tools/test_code_execution.py tests/tools/test_docker_environment.py tests/tools/test_credential_files.py tests/agent/test_prompt_builder.py tests/integration/test_execution_write_confinement.py -v --timeout=0— 56 passed, 1 skipped.scripts/check-windows-footguns.pyandgit diff --checkpassed.Not in scope
Native local, SSH, Singularity, Modal, managed Modal, Daytona, and Vercel Sandbox confinement adapters remain explicit unsupported cases for workspace scope. This change does not alter FileSyncManager or broker delivery roots, so private container output remains unpublished.
Upstream
Refs #36645.