mirrors/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-07-31 19:16:29 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

fix(file_ops): harden new-file umask chmod for portability

Browse source 
Follow-ups on top of #70888's cherry-picked fix:

- Replace the $((0666 & ~0$u)) shell arithmetic with POSIX who-less
  'chmod "=rw"'. zsh (reachable via _find_bash's $SHELL fallback on
  bash-less hosts) parses leading-zero constants as decimal and silently
  chmods a garbage mode (e.g. 0210); the symbolic form is spec-identical
  across bash/dash/busybox-ash/zsh and degrades to mktemp's 0600
  (pre-fix behavior) rather than corrupting perms if chmod rejects it.
- Move the new-file chmod after the content stream so the temp file
  stays owner-writable while cat runs.
- Run the chmod on a '[ ! -e "$t" ]' check after cat instead of the
  stat/else branch, keeping the overwrite path untouched.
- Update the stale perms comment #70856 called out (new files did NOT
  land with default umask perms pre-fix).
- Tests: select the atomic-write script by content instead of call
  order (the previous last-call capture only worked because the bare
  MagicMock's falsy-exit early return suppressed later execs), assert
  behavior at explicit umasks 0022/0002/0077 via parametrize, add an
  overwrite mode-preservation regression guard, and dedupe the
  real-subprocess env fake into make_real_subprocess_env() shared with
  TestSearchFilesFallbackHiddenPaths.

(webtecnica's email mapping already exists in contributors/emails/ on
current main; the PR's check-attribution red was stale-base only.)

# Conflicts:
#	tests/tools/test_file_operations.py
This commit is contained in:
kshitijk4poor 2026-07-30 20:40:29 +05:00 • committed by kshitij
parent f3cda0ceb1
commit 98105f31f4
2 changed files with 73 additions and 48 deletions
Download patch file Download diff file Expand all files Collapse all files

103
tests/tools/test_file_operations.py
View file

@ -242,6 +242,38 @@ def file_ops(mock_env):
return ShellFileOperations(mock_env)
def make_real_subprocess_env(cwd: str, include_stderr: bool = False) -> MagicMock:
"""Mock env whose execute() runs the command in a real subprocess.
For tests that need the generated shell scripts to actually run
(search fallback, atomic-write permissions) instead of being
intercepted by a bare MagicMock. ``include_stderr`` folds stderr
into ``output`` for tests that surface shell error text; leave it
off for tests that parse structured stdout (e.g. find results).
"""
env = MagicMock()
env.cwd = cwd
def execute(command, **kwargs):
completed = subprocess.run(
command,
shell=True,
text=True,
capture_output=True,
input=kwargs.get("stdin_data"),
)
output = completed.stdout
if include_stderr:
output += completed.stderr
return {
"output": output,
"returncode": completed.returncode,
}
env.execute = execute
return env
class TestShellFileOpsHelpers:
def test_normalize_read_pagination_clamps_invalid_values(self):
assert normalize_read_pagination(offset=0, limit=0) == (1, 1)
@ -393,23 +425,7 @@ class TestSearchPathValidation:
class TestSearchFilesFallbackHiddenPaths:
def _make_env(self):
env = MagicMock()
env.cwd = "/"
def execute(command, **kwargs):
completed = subprocess.run(
command,
shell=True,
text=True,
capture_output=True,
)
return {
"output": completed.stdout,
"returncode": completed.returncode,
}
env.execute = execute
return env
return make_real_subprocess_env("/")
def test_hidden_root_with_hidden_ancestor_includes_files(self, tmp_path, monkeypatch):
"""Fallback find should include visible files when path is inside hidden root."""
@ -561,42 +577,41 @@ class _DeletedTestGitBaselineCheck:
class TestAtomicWriteNewFilePermissions:
"""_atomic_write should apply umask-default perms to new files (not 0600)."""
def test_new_file_gets_umask_default_permissions(self, tmp_path):
@pytest.mark.parametrize("test_umask", [0o022, 0o002, 0o077])
def test_new_file_gets_umask_default_permissions(self, tmp_path, test_umask):
"""Newly created file should get umask-computed perms, not mktemp's 0600.
Uses a real subprocess so the shell script actually runs.
"""
env = MagicMock()
env.cwd = str(tmp_path)
def execute(command, **kwargs):
completed = subprocess.run(
command,
shell=True,
text=True,
capture_output=True,
input=kwargs.get("stdin_data"),
)
return {
"output": completed.stdout + completed.stderr,
"returncode": completed.returncode,
}
env.execute = execute
ops = ShellFileOperations(env)
ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path)))
dest = tmp_path / "new_file.txt"
assert not dest.exists()
result = ops.write_file(str(dest), "test content\n")
assert result.error is None, f"write failed: {result.error}"
assert dest.exists()
old_umask = os.umask(test_umask)
try:
result = ops.write_file(str(dest), "test content\n")
finally:
os.umask(old_umask)
# Compute expected mode: 0666 & ~umask
current_umask = os.umask(0)
os.umask(current_umask) # restore
expected_mode = 0o666 & ~current_umask
assert result.error is None, f"write failed: {result.error}"
assert dest.read_text() == "test content\n"
expected_mode = 0o666 & ~test_umask
actual_mode = dest.stat().st_mode & 0o777
assert actual_mode == expected_mode, (
f"Expected mode {expected_mode:04o} (umask {current_umask:04o}), "
f"Expected mode {expected_mode:04o} (umask {test_umask:04o}), "
f"got {actual_mode:04o}"
)
def test_overwrite_still_preserves_existing_mode(self, tmp_path):
"""The new-file branch must not disturb the overwrite path's
mode preservation (e.g. an executable script stays 0755)."""
ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path)))
dest = tmp_path / "existing.sh"
dest.write_text("#!/bin/sh\n")
dest.chmod(0o755)
result = ops.write_file(str(dest), "#!/bin/sh\necho updated\n")
assert result.error is None, f"write failed: {result.error}"
assert dest.read_text() == "#!/bin/sh\necho updated\n"
assert dest.stat().st_mode & 0o777 == 0o755

18
tools/file_operations.py
View file

@ -1005,7 +1005,16 @@ class ShellFileOperations(FileOperations):
# - `chmod --reference` is GNU-only, so we read the octal mode with
# `stat` (GNU `-c%a` or BSD `-f%Lp`) and `chmod` it explicitly;
# silent best-effort — a perms-copy failure must not abort the
# write, the file still lands with default umask perms.
# write (the file then lands at mktemp's 0600, same as pre-fix).
# - brand-new targets get `chmod "=rw"` — the POSIX who-less
# symbolic form, which sets rw minus the process umask (e.g.
# 0644 under umask 022) instead of mktemp's hardcoded 0600
# (#70856). Deliberately NOT shell arithmetic on `$(umask)`:
# zsh (reachable via _find_bash's $SHELL fallback) parses
# leading-zero constants as decimal and silently computes a
# garbage mode, while `chmod "=rw"` is spec-identical in
# bash/dash/ash/zsh and degrades to 0600 (pre-fix behavior)
# if an exotic chmod rejects it.
# - `trap ... EXIT` guarantees the temp is removed on every error
# path (cat failure, mv failure, signal) but NOT after a
# successful mv (the temp no longer exists by then).
@ -1022,11 +1031,12 @@ class ShellFileOperations(FileOperations):
'if [ -e "$t" ]; then '
'm="$(stat -c%a "$t" 2>/dev/null || stat -f%Lp "$t" 2>/dev/null || true)"; '
'[ -n "$m" ] && chmod "$m" "$tmp" 2>/dev/null || true; '
# new file: apply umask-computed default instead of mktemp's 0600
'else '
'u="$(umask)"; chmod $(printf '"'"'%04o'"'"' $((0666 & ~0$u))) "$tmp" 2>/dev/null || true; '
"fi; "
'cat > "$tmp"; '
# new file: umask-default perms instead of mktemp's 0600 (#70856).
# Runs AFTER cat so a write-masking umask can't EACCES the stream;
# quoted "=rw" so zsh doesn't =word-expand it.
'if [ ! -e "$t" ]; then chmod "=rw" "$tmp" 2>/dev/null || true; fi; '
'mv -f "$tmp" "$t"; '
"trap - EXIT"
)