Skip to content

fix: guard json.loads() and use atomic writes for persistent state - #29019

Open
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/guard-json-loads-and-atomic-writes
Open

fix: guard json.loads() and use atomic writes for persistent state#29019
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/guard-json-loads-and-atomic-writes

Conversation

@annguyenNous

Copy link
Copy Markdown
Contributor

Problem

Multiple files have unguarded json.loads() calls that crash on malformed/non-JSON responses, and several persistent state files use non-atomic write_text() that can corrupt on crash.

Fix

JSON decode guards (5 files):

  • weixin.py: _api_post() and _api_get() now catch json.JSONDecodeError and raise descriptive RuntimeError
  • browser_cdp_tool.py: Malformed CDP WebSocket frames are skipped instead of crashing the attach-to-target loop
  • osv_check.py: Non-JSON OSV API responses (502/503 HTML) return [] instead of crashing
  • api_server.py: Corrupted SQLite cache entries return None instead of propagating JSONDecodeError

Atomic writes (6 files):

  • skills_hub.py: HubLockFile and HubTapFile now use atomic_json_write() (temp+fsync+replace)
  • model_metadata.py: Context length cache uses atomic_yaml_write()
  • delivery.py: Cron delivery output uses tempfile+fsync+os.replace
  • run.py: Voice mode preferences use atomic_json_write()
  • environments/base.py: JSON store uses atomic_json_write()
  • checkpoint_manager.py: Project metadata uses atomic_json_write()

Resource management (1 file):

  • vision_tools.py: PIL Image.open() handle now closed in finally block to prevent fd leak during resize loops

Before vs After

File Before After
weixin.py API calls json.JSONDecodeError crash on non-200 HTML RuntimeError with context
CDP WebSocket Crash on malformed frame Skip and continue
osv_check.py Crash on 502 HTML body Return empty list
api_server.py cache Crash on corrupted SQLite Return None (cache miss)
skills_hub.py writes Partial write on crash Atomic replace
model_metadata.py cache Corrupted YAML on crash Atomic replace
vision_tools.py PIL fd leak until GC Explicit close in finally

Tests

  • All 11 modified files pass py_compile syntax check
  • Zero behavioral changes for happy paths — only error handling improved

JSON decode guards (5 files):
- weixin.py: wrap _api_post/_api_get json.loads in try/except
- browser_cdp_tool.py: skip malformed CDP WebSocket frames
- osv_check.py: return [] on non-JSON OSV API responses
- api_server.py: return None on corrupted SQLite cache entries

Atomic writes (6 files):
- skills_hub.py: use atomic_json_write for lock/taps files
- model_metadata.py: use atomic_yaml_write for context cache
- delivery.py: use tempfile+fsync+os.replace for delivery output
- run.py: use atomic_json_write for voice mode preferences
- environments/base.py: use atomic_json_write for JSON store
- checkpoint_manager.py: use atomic_json_write for project metadata

Resource management (1 file):
- vision_tools.py: close PIL Image in finally block to prevent fd leak

@teknium1 teknium1 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.

Thanks for collecting these hardening fixes in one place. Several of the underlying problems are still present on current main, but this branch needs salvage work before it is safe to carry forward.

Problems

  • tools/vision_tools.py is stale against current main. Current origin/main has _resize_image_for_vision(..., max_dimension=...) and requires both the byte cap and _dims_ok(...) before returning (tools/vision_tools.py:372, tools/vision_tools.py:502 on origin/main). The PR branch drops that parameter/guard and returns on len(candidate) <= max_base64_bytes at PR line tools/vision_tools.py:391, which would regress the newer dimension-cap fix.
  • The OSV 502/503 behavior is incomplete. In the PR branch, the new guard starts after urllib.request.urlopen(...) (tools/osv_check.py:150-154), but non-2xx HTTP responses commonly raise before resp.read() is reached.
  • No behavioral tests were added; the PR body only reports py_compile coverage.

Suggested changes

  • Reapply the vision fd-close fix on top of current main while preserving max_dimension and _dims_ok.
  • Wrap the OSV urlopen call itself if transient HTML/non-JSON failures should return [].
  • Add focused tests for the malformed JSON and atomic-write paths touched here.

Automated hermes-sweeper review.

Comment thread tools/vision_tools.py
img.save(buf, **save_kwargs)
encoded = base64.b64encode(buf.getvalue()).decode("ascii")
candidate = f"data:{out_mime};base64,{encoded}"
if len(candidate) <= max_base64_bytes:

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.

This branch is stale against current main: the current helper also gates success on max_dimension via _dims_ok(...). Salvage should keep that dimension check while adding the image close/finally behavior.

Comment thread tools/osv_check.py
with urllib.request.urlopen(req, timeout=_TIMEOUT) as resp:
result = json.loads(resp.read())
try:
result = json.loads(resp.read())

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.

This catches malformed JSON after a successful urlopen, but 502/503 responses often raise HTTPError at urlopen before this line. If the goal is to return [] for transient HTML service failures, wrap urlopen too.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/browser Browser automation (CDP, Playwright) tool/skills Skills system (list, view, manage) tool/vision Vision analysis and image generation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants