diff --git a/config.py b/config.py index 26c81fb..a071d2c 100644 --- a/config.py +++ b/config.py @@ -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") diff --git a/services/runtime_config.py b/services/runtime_config.py index 987bbe5..5c364a2 100644 --- a/services/runtime_config.py +++ b/services/runtime_config.py @@ -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, diff --git a/services/state_dir.py b/services/state_dir.py index 10ad78b..be6207d 100644 --- a/services/state_dir.py +++ b/services/state_dir.py @@ -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() diff --git a/tests/test_r175_pack_version_fallback.py b/tests/test_r175_pack_version_fallback.py index 4aef3ca..41313e4 100644 --- a/tests/test_r175_pack_version_fallback.py +++ b/tests/test_r175_pack_version_fallback.py @@ -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): diff --git a/tests/test_r176_import_safe_config.py b/tests/test_r176_import_safe_config.py new file mode 100644 index 0000000..8d9c9fb --- /dev/null +++ b/tests/test_r176_import_safe_config.py @@ -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()