fix(acp): use tempfile.gettempdir() in workspace auto-approve
#28063 fixed the macOS `/tmp`→`/private/tmp` symlink issue by checking the RAW path (pre-resolve) against startswith('/tmp/'). That works on Linux + macOS but not on Windows — Path('/tmp/foo').resolve() returns C:\\tmp\\foo and isn't the real Windows temp anyway. Replace the hardcoded '/tmp/' prefix with Path(tempfile.gettempdir()). resolve() + Path.relative_to() — same idiom as the cwd branch just below. Works correctly on Linux (/tmp), macOS (/private/var/folders/...), and Windows (%LOCALAPPDATA%\\Temp). Test rewritten to use tempfile.gettempdir() so the assertion exercises the same code path on every platform. Conflict against the just-merged #28063 (raw_path approach) resolved by replacing the whole raw_path block — tempfile.gettempdir() is strictly better than that intermediate fix. Salvage of #28262 by @Zyrixtrex.
This commit is contained in:
@@ -10,6 +10,7 @@ from __future__ import annotations
|
|||||||
import asyncio
|
import asyncio
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
|
import tempfile
|
||||||
from concurrent.futures import TimeoutError as FutureTimeout
|
from concurrent.futures import TimeoutError as FutureTimeout
|
||||||
from contextvars import ContextVar, Token
|
from contextvars import ContextVar, Token
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
@@ -154,16 +155,19 @@ def should_auto_approve_edit(proposal: EditProposal, policy: str, cwd: str | Non
|
|||||||
policy = str(policy or AUTO_APPROVE_ASK).strip()
|
policy = str(policy or AUTO_APPROVE_ASK).strip()
|
||||||
if policy == AUTO_APPROVE_ASK or _is_sensitive_auto_approve_path(proposal.path):
|
if policy == AUTO_APPROVE_ASK or _is_sensitive_auto_approve_path(proposal.path):
|
||||||
return False
|
return False
|
||||||
raw_path = Path(proposal.path).expanduser()
|
path = Path(proposal.path).expanduser().resolve(strict=False)
|
||||||
# resolve() follows symlinks — on macOS /tmp → /private/tmp, so the
|
|
||||||
# resolved form never starts with "/tmp/". Use raw_path for the /tmp
|
|
||||||
# check and the resolved form only for the cwd relative_to() test.
|
|
||||||
path = raw_path.resolve(strict=False)
|
|
||||||
if policy == AUTO_APPROVE_SESSION:
|
if policy == AUTO_APPROVE_SESSION:
|
||||||
return True
|
return True
|
||||||
if policy == AUTO_APPROVE_WORKSPACE:
|
if policy == AUTO_APPROVE_WORKSPACE:
|
||||||
if str(raw_path).startswith("/tmp/"):
|
# `/tmp` is the POSIX path but tempfile.gettempdir() is the real one on
|
||||||
|
# every platform: `/private/tmp` on macOS (because `/tmp` is a symlink
|
||||||
|
# and Path.resolve() follows it) and the per-user Temp dir on Windows.
|
||||||
|
tmp_root = Path(tempfile.gettempdir()).resolve(strict=False)
|
||||||
|
try:
|
||||||
|
path.relative_to(tmp_root)
|
||||||
return True
|
return True
|
||||||
|
except ValueError:
|
||||||
|
pass
|
||||||
if cwd:
|
if cwd:
|
||||||
root = Path(cwd).expanduser().resolve(strict=False)
|
root = Path(cwd).expanduser().resolve(strict=False)
|
||||||
try:
|
try:
|
||||||
|
|||||||
@@ -3,6 +3,7 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import json
|
import json
|
||||||
|
import tempfile
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
from acp_adapter.edit_approval import (
|
from acp_adapter.edit_approval import (
|
||||||
@@ -183,7 +184,10 @@ def test_patch_replace_approval_request_includes_full_file_diff(tmp_path):
|
|||||||
|
|
||||||
def test_workspace_auto_approval_allows_workspace_and_tmp_but_not_sensitive(tmp_path):
|
def test_workspace_auto_approval_allows_workspace_and_tmp_but_not_sensitive(tmp_path):
|
||||||
workspace_file = tmp_path / "src.py"
|
workspace_file = tmp_path / "src.py"
|
||||||
tmp_file = Path("/tmp/hermes-acp-auto-approve-test.txt")
|
# Use tempfile.gettempdir() so this test exercises the same code path on
|
||||||
|
# Linux (`/tmp`), macOS (`/private/var/folders/...`) and Windows
|
||||||
|
# (`%LOCALAPPDATA%\Temp`). Before the fix this branch only worked on Linux.
|
||||||
|
tmp_file = Path(tempfile.gettempdir()) / "hermes-acp-auto-approve-test.txt"
|
||||||
env_file = tmp_path / ".env"
|
env_file = tmp_path / ".env"
|
||||||
|
|
||||||
assert should_auto_approve_edit(
|
assert should_auto_approve_edit(
|
||||||
|
|||||||
Reference in New Issue
Block a user