Skip to content

fix: guard yaml.safe_load, flock unlock, TOCTOU races, and atomic writes - #28018

Closed
vanthinh6886 wants to merge 1 commit into
NousResearch:mainfrom
vanthinh6886:fix/yaml-safeload-flock-toctou-atomic-writes
Closed

fix: guard yaml.safe_load, flock unlock, TOCTOU races, and atomic writes#28018
vanthinh6886 wants to merge 1 commit into
NousResearch:mainfrom
vanthinh6886:fix/yaml-safeload-flock-toctou-atomic-writes

Conversation

@vanthinh6886

Copy link
Copy Markdown
Contributor

Summary

Fixes 4 classes of bugs across 9 files:

1. yaml.safe_load() crash on empty config (HIGH)

File: trajectory_compressor.py
yaml.safe_load() returns None for empty files. The subsequent if 'tokenizer' in data raises TypeError. Fix: add or {} fallback.

2. Unguarded fcntl.flock(LOCK_UN) in finally blocks (MEDIUM x6)

Files: cron/scheduler.py, hermes_cli/auth.py, agent/shell_hooks.py, tools/skill_usage.py, tools/environments/file_sync.py, tools/memory_tool.py

If flock(LOCK_UN) raises OSError, fd.close() is skipped, leaking the fd and holding the lock forever. The msvcrt branches already had try/except; the fcntl branches did not. Fix: wrap in try/except (OSError, IOError): pass.

3. TOCTOU race in copilot_acp_client.py (MEDIUM)

path.exists() followed by path.read_text() with no exception handling. Fix: use try/except FileNotFoundError.

4. Non-atomic cache write in sticker_cache.py (MEDIUM)

Path.write_text() leaves truncated JSON on crash. Fix: tempfile + fsync + os.replace() (atomic).

Test Plan

  • All 9 modified files pass python3 -m py_compile
  • No new lint errors introduced
  • CI passes

1. trajectory_compressor.py: yaml.safe_load() returns None on empty
   files, crashing with TypeError on `if 'tokenizer' in data`. Fix by
   adding `or {}` fallback. (HIGH — blocks startup with empty config)

2. 6 files with fcntl.flock(LOCK_UN) in finally blocks without
   try/except: cron/scheduler.py, hermes_cli/auth.py,
   agent/shell_hooks.py, tools/skill_usage.py,
   tools/environments/file_sync.py, tools/memory_tool.py. If unlock
   raises OSError, fd.close() is skipped and the lock is held forever.
   The msvcrt branches already had try/except; the fcntl branches did
   not. Fix by wrapping in try/except (OSError, IOError): pass.

3. agent/copilot_acp_client.py line 639: TOCTOU race — path.exists()
   followed by path.read_text() with no try/except. If file is deleted
   between the check and the read, FileNotFoundError propagates. Fix
   by using try/except FileNotFoundError.

4. gateway/sticker_cache.py: non-atomic write via Path.write_text()
   can leave truncated JSON on crash, causing JSONDecodeError on next
   load. Fix by writing to tempfile + fsync + os.replace (atomic).
@BoardJames-Bot

Copy link
Copy Markdown

BoardJames triage: current branch-local blocker is check-attribution, not the code change itself. CI reports the new author email vanthinh6886@gmail.com (vanthinh6886) is missing from scripts/release.py AUTHOR_MAP; GitHub email search returned no public login for that email, but the PR author/login is vanthinh6886, so the narrow fix is to add:

"vanthinh6886@gmail.com": "vanthinh6886",

to AUTHOR_MAP and rerun checks. All other completed checks are green so far (ruff + ty, ruff enforcement, Windows footguns, nix ubuntu/macos, supply-chain, common-ancestor, e2e, build-amd64). Tests / test and build-arm64 were still running when I checked, so I’m not attributing those pending states to the branch yet.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cron Cron scheduler and job management tool/memory Memory tool and memory providers P2 Medium — degraded but workaround exists labels May 18, 2026

@outsourc-e outsourc-e left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed locally in a clean worktree. I spot-checked the touched paths and compiled all 9 modified files. The changes are narrowly defensive: safe_load fallback on empty config, fail-safe unlock cleanup, TOCTOU hardening, and atomic sticker-cache writes. I did not find a new security concern in the implementation.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #28593 (cherry-picked onto current main with your authorship preserved via rebase-merge — commit 62573f4). Thanks for the contribution!

@teknium1 teknium1 closed this May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants