mirror of
https://github.com/rookiestar28/ComfyUI-OpenClaw.git
synced 2026-08-14 00:48:07 +00:00
fix(config): defer bootstrap side effects
This commit is contained in:
@@ -127,24 +127,39 @@ LEGACY2_ENV_API_KEY = GENERIC_LLM_API_KEY_ENV_KEYS[2]
|
||||
# Data directory (R11: use portable state directory)
|
||||
try:
|
||||
# Prefer package-relative import (ComfyUI loads custom nodes by file loader)
|
||||
from .services.state_dir import get_log_path, get_state_dir # type: ignore
|
||||
from .services.state_dir import ( # type: ignore
|
||||
get_log_path,
|
||||
get_state_dir,
|
||||
peek_log_path,
|
||||
peek_state_dir,
|
||||
)
|
||||
except Exception:
|
||||
try:
|
||||
# Fallback for unit tests / direct sys.path imports
|
||||
from services.state_dir import get_log_path, get_state_dir
|
||||
from services.state_dir import (
|
||||
get_log_path,
|
||||
get_state_dir,
|
||||
peek_log_path,
|
||||
peek_state_dir,
|
||||
)
|
||||
except Exception:
|
||||
get_state_dir = None
|
||||
get_log_path = None
|
||||
peek_state_dir = None
|
||||
peek_log_path = None
|
||||
|
||||
if get_state_dir and get_log_path:
|
||||
DATA_DIR = get_state_dir()
|
||||
LOG_FILE = get_log_path()
|
||||
if peek_state_dir and peek_log_path:
|
||||
DATA_DIR = peek_state_dir()
|
||||
LOG_FILE = peek_log_path()
|
||||
else:
|
||||
# Last-resort fallback during early import or if state_dir is unavailable
|
||||
PACK_DIR = os.path.dirname(os.path.abspath(__file__))
|
||||
DATA_DIR = os.path.join(PACK_DIR, "data")
|
||||
LOG_FILE = os.path.join(DATA_DIR, "openclaw.log")
|
||||
|
||||
_IMPORT_DATA_DIR = DATA_DIR
|
||||
_IMPORT_LOG_FILE = LOG_FILE
|
||||
|
||||
# IMPORTANT: startup log truncation must run once per process.
|
||||
# Multiple module-level loggers call setup_logger(); repeated truncation would
|
||||
# erase fresh logs emitted after the first logger initialization.
|
||||
@@ -180,6 +195,16 @@ def _maybe_truncate_log_on_start(logger: logging.Logger) -> None:
|
||||
logger.warning(f"Failed to truncate startup log file {LOG_FILE}: {e}")
|
||||
|
||||
|
||||
def _ensure_log_targets() -> tuple[str, str]:
|
||||
global DATA_DIR, LOG_FILE
|
||||
if DATA_DIR != _IMPORT_DATA_DIR or LOG_FILE != _IMPORT_LOG_FILE:
|
||||
return DATA_DIR, LOG_FILE
|
||||
if get_state_dir and get_log_path:
|
||||
DATA_DIR = get_state_dir()
|
||||
LOG_FILE = get_log_path()
|
||||
return DATA_DIR, LOG_FILE
|
||||
|
||||
|
||||
class RedactedFormatter(logging.Formatter):
|
||||
"""
|
||||
Custom formatter to redact sensitive information (like API keys) from logs.
|
||||
@@ -223,6 +248,7 @@ def setup_logger(name: str = "ComfyUI-OpenClaw") -> logging.Logger:
|
||||
|
||||
# Only add handler if not already added to avoid duplicates on reload
|
||||
if not logger.handlers:
|
||||
data_dir, log_file = _ensure_log_targets()
|
||||
_maybe_truncate_log_on_start(logger)
|
||||
api_key = get_api_key()
|
||||
sensitive = [api_key] if api_key else []
|
||||
@@ -237,9 +263,9 @@ def setup_logger(name: str = "ComfyUI-OpenClaw") -> logging.Logger:
|
||||
|
||||
# File handler with rotation (5MB, 3 backups)
|
||||
try:
|
||||
os.makedirs(DATA_DIR, exist_ok=True)
|
||||
os.makedirs(data_dir, exist_ok=True)
|
||||
file_handler = RotatingFileHandler(
|
||||
LOG_FILE,
|
||||
log_file,
|
||||
maxBytes=5 * 1024 * 1024, # 5MB
|
||||
backupCount=3,
|
||||
encoding="utf-8",
|
||||
@@ -255,5 +281,5 @@ def setup_logger(name: str = "ComfyUI-OpenClaw") -> logging.Logger:
|
||||
return logger
|
||||
|
||||
|
||||
# Global config accessor if needed
|
||||
logger = setup_logger()
|
||||
# Global logger handle for compatibility; actual handler bootstrap is lazy.
|
||||
logger = logging.getLogger("ComfyUI-OpenClaw")
|
||||
|
||||
@@ -261,9 +261,12 @@ except ImportError:
|
||||
# Config file location (under state dir)
|
||||
try:
|
||||
# Prefer package-relative imports when running as a ComfyUI custom node pack.
|
||||
from .state_dir import get_state_dir
|
||||
# CRITICAL: config path resolution here must stay import-safe.
|
||||
# Calling get_state_dir() at import time recreates the state dir during plain
|
||||
# module import and breaks lazy bootstrap guarantees in config.py.
|
||||
from .state_dir import peek_state_dir
|
||||
|
||||
CONFIG_FILE = os.path.join(get_state_dir(), "config.json")
|
||||
CONFIG_FILE = os.path.join(peek_state_dir(), "config.json")
|
||||
from .providers.catalog import (
|
||||
PROVIDER_CATALOG,
|
||||
get_default_public_llm_hosts,
|
||||
@@ -274,9 +277,9 @@ try:
|
||||
except ImportError:
|
||||
try:
|
||||
# Fallback for direct sys.path imports (unit tests / scripts)
|
||||
from services.state_dir import get_state_dir # type: ignore
|
||||
from services.state_dir import peek_state_dir # type: ignore
|
||||
|
||||
CONFIG_FILE = os.path.join(get_state_dir(), "config.json")
|
||||
CONFIG_FILE = os.path.join(peek_state_dir(), "config.json")
|
||||
from services.providers.catalog import ( # type: ignore
|
||||
PROVIDER_CATALOG,
|
||||
get_default_public_llm_hosts,
|
||||
|
||||
+33
-14
@@ -44,6 +44,26 @@ def _get_user_data_dir(subdir: Optional[str] = None) -> str:
|
||||
return os.path.join(base, subdir)
|
||||
|
||||
|
||||
def _resolve_state_dir_path() -> str:
|
||||
"""Resolve the canonical state-dir path without creating it."""
|
||||
env_dir = os.environ.get(STATE_DIR_ENV) or os.environ.get(LEGACY_STATE_DIR_ENV)
|
||||
if env_dir:
|
||||
return os.path.abspath(env_dir)
|
||||
|
||||
new_dir = _get_user_data_dir(STATE_DIR_NAME)
|
||||
legacy_dir = _get_user_data_dir(LEGACY_STATE_DIR_NAME)
|
||||
return (
|
||||
legacy_dir
|
||||
if (os.path.exists(legacy_dir) and not os.path.exists(new_dir))
|
||||
else new_dir
|
||||
)
|
||||
|
||||
|
||||
def peek_state_dir() -> str:
|
||||
"""Return the canonical state-dir path without creating directories."""
|
||||
return _resolve_state_dir_path()
|
||||
|
||||
|
||||
def get_state_dir() -> str:
|
||||
"""
|
||||
Get the canonical state directory for all writable data.
|
||||
@@ -58,20 +78,7 @@ def get_state_dir() -> str:
|
||||
Returns:
|
||||
Absolute path to state directory.
|
||||
"""
|
||||
# Check for explicit override
|
||||
env_dir = os.environ.get(STATE_DIR_ENV) or os.environ.get(LEGACY_STATE_DIR_ENV)
|
||||
if env_dir:
|
||||
state_dir = os.path.abspath(env_dir)
|
||||
else:
|
||||
# Prefer the new default directory name, but preserve existing legacy state
|
||||
# if it already exists (rename shouldn't silently "lose" user config/logs).
|
||||
new_dir = _get_user_data_dir(STATE_DIR_NAME)
|
||||
legacy_dir = _get_user_data_dir(LEGACY_STATE_DIR_NAME)
|
||||
state_dir = (
|
||||
legacy_dir
|
||||
if (os.path.exists(legacy_dir) and not os.path.exists(new_dir))
|
||||
else new_dir
|
||||
)
|
||||
state_dir = _resolve_state_dir_path()
|
||||
|
||||
# Ensure directory exists with appropriate permissions
|
||||
if not os.path.exists(state_dir):
|
||||
@@ -90,6 +97,18 @@ def get_state_dir() -> str:
|
||||
return state_dir
|
||||
|
||||
|
||||
def peek_log_path() -> str:
|
||||
"""Return the canonical log-file path without creating directories."""
|
||||
state_dir = peek_state_dir()
|
||||
new_path = os.path.join(state_dir, "openclaw.log")
|
||||
legacy_path = os.path.join(state_dir, "moltbot.log")
|
||||
return (
|
||||
legacy_path
|
||||
if (os.path.exists(legacy_path) and not os.path.exists(new_path))
|
||||
else new_path
|
||||
)
|
||||
|
||||
|
||||
def get_log_path() -> str:
|
||||
"""Get the path for the log file."""
|
||||
state_dir = get_state_dir()
|
||||
|
||||
@@ -30,9 +30,7 @@ class TestR175PackVersionFallback(unittest.TestCase):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
missing = Path(tmpdir) / "missing.toml"
|
||||
self.assertIsNone(
|
||||
config._read_pyproject_version_from_path(
|
||||
missing, prefer_tomllib=False
|
||||
)
|
||||
config._read_pyproject_version_from_path(missing, prefer_tomllib=False)
|
||||
)
|
||||
|
||||
def test_repo_pack_version_matches_pyproject_source_of_truth(self):
|
||||
|
||||
@@ -0,0 +1,105 @@
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
import textwrap
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
from services import state_dir
|
||||
|
||||
ROOT = Path(__file__).resolve().parents[1]
|
||||
|
||||
|
||||
class TestR176ImportSafeConfig(unittest.TestCase):
|
||||
def _run_python(self, script: str, *, env: dict[str, str]) -> subprocess.CompletedProcess:
|
||||
return subprocess.run(
|
||||
[sys.executable, "-c", script],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
cwd=str(ROOT),
|
||||
env=env,
|
||||
check=False,
|
||||
)
|
||||
|
||||
def test_peek_helpers_do_not_create_state_directory(self):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
target = Path(tmpdir) / "state"
|
||||
with patch.dict(
|
||||
os.environ,
|
||||
{"OPENCLAW_STATE_DIR": str(target)},
|
||||
clear=False,
|
||||
):
|
||||
self.assertFalse(target.exists())
|
||||
self.assertEqual(state_dir.peek_state_dir(), str(target))
|
||||
self.assertFalse(target.exists())
|
||||
self.assertEqual(
|
||||
state_dir.peek_log_path(),
|
||||
str(target / "openclaw.log"),
|
||||
)
|
||||
self.assertFalse(target.exists())
|
||||
|
||||
def test_importing_config_does_not_create_state_dir_or_log_file(self):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
state_root = Path(tmpdir) / "state"
|
||||
script = textwrap.dedent(
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
from pathlib import Path
|
||||
import config
|
||||
|
||||
state_root = Path(os.environ["OPENCLAW_STATE_DIR"])
|
||||
print(json.dumps({
|
||||
"state_exists": state_root.exists(),
|
||||
"log_exists": (state_root / "openclaw.log").exists(),
|
||||
"handler_count": len(config.logger.handlers),
|
||||
}))
|
||||
"""
|
||||
)
|
||||
env = dict(os.environ)
|
||||
env["OPENCLAW_STATE_DIR"] = str(state_root)
|
||||
result = self._run_python(script, env=env)
|
||||
self.assertEqual(result.returncode, 0, msg=result.stdout + result.stderr)
|
||||
payload = json.loads(result.stdout.strip())
|
||||
self.assertFalse(payload["state_exists"])
|
||||
self.assertFalse(payload["log_exists"])
|
||||
self.assertEqual(payload["handler_count"], 0)
|
||||
|
||||
def test_setup_logger_creates_state_dir_and_log_file_on_first_use(self):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
state_root = Path(tmpdir) / "state"
|
||||
script = textwrap.dedent(
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
from pathlib import Path
|
||||
import config
|
||||
|
||||
logger = config.setup_logger("r176.first_use")
|
||||
logger.info("lazy-init-ok")
|
||||
state_root = Path(os.environ["OPENCLAW_STATE_DIR"])
|
||||
log_file = state_root / "openclaw.log"
|
||||
print(json.dumps({
|
||||
"state_exists": state_root.exists(),
|
||||
"log_exists": log_file.exists(),
|
||||
"handler_count": len(logger.handlers),
|
||||
"contains_message": log_file.exists() and "lazy-init-ok" in log_file.read_text(encoding="utf-8"),
|
||||
}))
|
||||
"""
|
||||
)
|
||||
env = dict(os.environ)
|
||||
env["OPENCLAW_STATE_DIR"] = str(state_root)
|
||||
result = self._run_python(script, env=env)
|
||||
self.assertEqual(result.returncode, 0, msg=result.stdout + result.stderr)
|
||||
payload = json.loads(result.stdout.strip())
|
||||
self.assertTrue(payload["state_exists"])
|
||||
self.assertTrue(payload["log_exists"])
|
||||
self.assertGreaterEqual(payload["handler_count"], 1)
|
||||
self.assertTrue(payload["contains_message"])
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user