From 01759ac02941a261007537713b9b3d8cb10f9df2 Mon Sep 17 00:00:00 2001 From: rookiestar28 <151893693+rookiestar28@users.noreply.github.com> Date: Fri, 6 Feb 2026 10:56:47 +0800 Subject: [PATCH] fix: E2E test --- .github/workflows/pre-commit.yml | 7 ++-- .pre-commit-config.yaml | 9 ++--- README.md | 5 --- api/routes.py | 53 +++++++++++++++++++++++++-- scripts/precommit_black_single.py | 35 ++++++++++++++++++ services/capabilities.py | 3 ++ services/diagnostics_flags.py | 9 +++-- tests/TEST_SOP.md | 59 +++++++++++++++---------------- tests/test_diagnostics_flags.py | 26 ++++++++------ 9 files changed, 150 insertions(+), 56 deletions(-) create mode 100644 scripts/precommit_black_single.py diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index cea0bd4..29758e4 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -23,8 +23,11 @@ jobs: with: python-version: "3.11" - - name: Install pre-commit - run: pip install pre-commit + - name: Install pre-commit (and formatters) + run: | + # Install black/isort explicitly so CI doesn't fail due to missing tools + # if a hook is configured to run via system python. + pip install pre-commit black==24.1.1 isort==5.13.2 - name: Run all pre-commit hooks run: pre-commit run --all-files --show-diff-on-failure diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 89e7f8b..b798e92 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -42,13 +42,14 @@ repos: - id: check-added-large-files args: ['--maxkb=500'] - # Python formatting (single-file execution to avoid multiprocessing socket issues) + # Python formatting (cross-platform, single-file runner to avoid black multiprocessing issues) - repo: local hooks: - id: black-single - name: black (single-file, no multiprocessing) - entry: bash -lc 'status=0; for f in "$@"; do python3 -m black --check --diff "$f" || status=$?; done; exit $status' -- - language: system + name: black (single-file) + entry: python scripts/precommit_black_single.py + language: python + additional_dependencies: ['black==24.1.1'] types: [python] # Remove --check --diff to auto-format diff --git a/README.md b/README.md index 7b633ef..a63d5bc 100644 --- a/README.md +++ b/README.md @@ -308,8 +308,3 @@ python3 -m unittest discover -s tests -p "test_*.py" ## Security Read `SECURITY.md` before exposing any endpoint beyond localhost. The project is designed to be secure-by-default (deny-by-default auth, SSRF protections, redaction, bounded outputs), but unsafe deployment can still create risk. - -## Project planning - -- `ROADMAP.md` tracks feature status and priorities. -- `.planning/` contains detailed plans and implementation records. diff --git a/api/routes.py b/api/routes.py index 40fa1aa..08ce3b8 100644 --- a/api/routes.py +++ b/api/routes.py @@ -44,7 +44,11 @@ if web is not None: llm_test_handler, ) from ..api.preflight_handler import inventory_handler, preflight_handler - from ..api.secrets import secrets_delete_handler, secrets_put_handler + from ..api.secrets import ( + secrets_delete_handler, + secrets_put_handler, + secrets_status_handler, + ) from ..api.webhook import webhook_handler from ..api.webhook_submit import webhook_submit_handler from ..api.webhook_validate import webhook_validate_handler @@ -52,7 +56,15 @@ if web is not None: # IMPORTANT: use PACK_VERSION / PACK_START_TIME from config. # Do NOT import VERSION or config_path (they do not exist) or route registration will fail. from ..config import LOG_FILE, PACK_NAME, PACK_START_TIME, PACK_VERSION + + # CRITICAL: These imports MUST remain present. + # If edited out, module-level placeholders stay as None and handlers raise at runtime + # (e.g., TypeError: 'NoneType' object is not callable), producing noisy aiohttp tracebacks. + from ..services.access_control import require_observability_access + from ..services.log_tail import tail_log + from ..services.metrics import metrics from ..services.rate_limit import check_rate_limit + from ..services.redaction import redact_text # IMPORTANT: services.trace does NOT expose a `trace` symbol. # Do not import `trace` here or route registration will fail. @@ -72,13 +84,21 @@ if web is not None: llm_test_handler, ) from api.preflight_handler import inventory_handler, preflight_handler - from api.secrets import secrets_delete_handler, secrets_put_handler + from api.secrets import ( + secrets_delete_handler, + secrets_put_handler, + secrets_status_handler, + ) from api.webhook import webhook_handler from api.webhook_submit import webhook_submit_handler from api.webhook_validate import webhook_validate_handler # IMPORTANT: keep PACK_* imports aligned with config.py (VERSION/config_path do not exist). from config import LOG_FILE, PACK_NAME, PACK_START_TIME, PACK_VERSION + from services.access_control import require_observability_access # type: ignore + from services.log_tail import tail_log # type: ignore + from services.metrics import metrics # type: ignore + from services.rate_limit import check_rate_limit # type: ignore from services.redaction import redact_text # type: ignore # IMPORTANT: services.trace does NOT expose a `trace` symbol. @@ -95,6 +115,29 @@ def check_dependency(module_name: str) -> bool: return False +def _ensure_observability_deps_ready() -> tuple[bool, str | None]: + """ + Defensive guard against a recurring class of regressions: + if the import block above is edited incorrectly, the module-level + placeholders stay as None and handlers raise TypeError at runtime. + """ + missing: list[str] = [] + if not callable(require_observability_access): + missing.append("require_observability_access") + if not callable(check_rate_limit): + missing.append("check_rate_limit") + if not callable(tail_log): + missing.append("tail_log") + if missing: + return ( + False, + "Backend not fully initialized (missing route dependencies: " + + ", ".join(missing) + + ").", + ) + return True, None + + async def health_handler(request: web.Request) -> web.Response: """ GET /openclaw/health (legacy: /moltbot/health) @@ -201,6 +244,9 @@ async def logs_tail_handler(request: web.Request) -> web.Response: """GET /moltbot/logs/tail - Returns the last N lines of the log file.""" if web is None: raise RuntimeError("aiohttp not available") + ok, init_error = _ensure_observability_deps_ready() + if not ok: + return web.json_response({"ok": False, "error": init_error}, status=500) # S14: Access Control allowed, error = require_observability_access(request) if not allowed: @@ -301,6 +347,9 @@ async def trace_handler(request: web.Request) -> web.Response: """GET /moltbot/trace/{prompt_id} - Returns trace_id and redacted timeline.""" if web is None: raise RuntimeError("aiohttp not available") + ok, init_error = _ensure_observability_deps_ready() + if not ok: + return web.json_response({"ok": False, "error": init_error}, status=500) allowed, error = require_observability_access(request) if not allowed: return web.json_response({"ok": False, "error": error}, status=403) diff --git a/scripts/precommit_black_single.py b/scripts/precommit_black_single.py new file mode 100644 index 0000000..42cae83 --- /dev/null +++ b/scripts/precommit_black_single.py @@ -0,0 +1,35 @@ +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + + +def _run_black_on_file(path: str) -> int: + # Run black one file at a time to avoid multiprocessing Manager/socket issues + # seen in some restricted environments. + res = subprocess.run( + [sys.executable, "-m", "black", "--check", "--diff", path], + stdout=sys.stdout, + stderr=sys.stderr, + text=False, + ) + return int(res.returncode) + + +def main(argv: list[str]) -> int: + # pre-commit passes filenames as argv; we format/check them individually. + status = 0 + for raw in argv[1:]: + p = Path(raw) + if not p.exists() or p.is_dir(): + continue + rc = _run_black_on_file(str(p)) + if rc != 0: + status = rc + return status + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv)) + diff --git a/services/capabilities.py b/services/capabilities.py index e9776aa..f9211fc 100644 --- a/services/capabilities.py +++ b/services/capabilities.py @@ -26,6 +26,9 @@ def get_capabilities() -> dict: "features": { "webhook_submit": True, "logs_tail": True, + # Legacy flag (kept for older frontends/tests). + # Do not remove without a migration window + frontend update. + "doctor": True, "job_monitor": True, "callback_delivery": True, "presets": True, diff --git a/services/diagnostics_flags.py b/services/diagnostics_flags.py index 563dbbf..b41a9cd 100644 --- a/services/diagnostics_flags.py +++ b/services/diagnostics_flags.py @@ -70,7 +70,9 @@ class ScopedLogger: 2. Redacts sensitive data before logging. """ - def __init__(self, logger: logging.Logger, subsystem: str, manager: DiagnosticsManager): + def __init__( + self, logger: logging.Logger, subsystem: str, manager: DiagnosticsManager + ): self._logger = logger self._subsystem = subsystem self._manager = manager @@ -88,18 +90,19 @@ class ScopedLogger: # Prepare message prefix = f"[DIAG:{self._subsystem}]" - + # Reduct data if present if data: safe_data = redact_dict_safe(data) # Serialize for clarity import json + try: # Use default str for non-serializable objects json_part = json.dumps(safe_data, default=str) except Exception: json_part = str(safe_data) - + self._logger.info(f"{prefix} {msg} | Data: {json_part}", **kwargs) else: self._logger.info(f"{prefix} {msg}", **kwargs) diff --git a/tests/TEST_SOP.md b/tests/TEST_SOP.md index 04f211f..24d8b6c 100644 --- a/tests/TEST_SOP.md +++ b/tests/TEST_SOP.md @@ -1,53 +1,52 @@ -# Test SOP +# Test SOP -This document defines the **mandatory test workflow** for this repo. Follow it **before every push** unless explicitly skipping tests for a scoped reason. +This document defines the **mandatory test workflow** for this repo. Run it **before every push** (unless you explicitly document why you’re skipping). ## Prerequisites -- Python 3.10+ -- Node.js 20+ -- `pre-commit` installed (`pip install pre-commit`) -- Playwright browsers installed (one-time): `npx playwright install chromium` +- Python 3.10+ (CI uses 3.10/3.11) +- Node.js 18+ (CI uses 20) +- `pre-commit` installed: `python -m pip install pre-commit` +- Frontend deps installed: `npm install` ## Required Pre-Push Workflow (Must Run) 1) Detect Secrets (baseline-based) -``` +```bash pre-commit run detect-secrets --all-files ``` 2) Run all pre-commit hooks -``` -pre-commit run --all-files +```bash +pre-commit run --all-files --show-diff-on-failure ``` -3) Frontend E2E (Playwright) +3) Backend unit tests (recommended; CI enforces) +```bash +MOLTBOT_STATE_DIR="$(pwd)/moltbot_state/_local_unit" python -m unittest discover -s tests -p "test_*.py" -v ``` -npm install + +4) Frontend E2E (Playwright; CI enforces) +```bash +# One-time browser install (recommended) +npx playwright install chromium + npm test ``` -## Optional (Local Developer Confidence) -Run only if your environment has Python deps installed. -``` -python3 -m unittest tests.test_checkpoints -v -python3 -m unittest tests.test_preflight -v -python3 -m unittest tests.test_checkpoints_api -v -python3 -m unittest tests.test_api_model_list -v -``` +For OS-specific E2E setup (Windows/WSL temp-dir shims), see `tests/E2E_TESTING_SOP.md`. -## CI Equivalence (What the pipeline enforces) -- Pre-commit hooks (all files) -- Playwright E2E (`npm test`) -- Import smoke tests +## WSL / Restricted Environments +If `pre-commit` fails due to cache permissions, run with a writable cache directory: +```bash +PRE_COMMIT_HOME=/tmp/pre-commit-cache pre-commit run --all-files --show-diff-on-failure +``` ## Troubleshooting Quick Fixes **Detect-secrets fails** -- Ensure `.secrets.baseline` is up to date. -- Replace real-looking secrets in docs/examples with ``. +- Update `.secrets.baseline` (or mark known false positives) and avoid real-looking secrets in docs/tests. -**Playwright fails to install** -- Ensure `npm` and `node` are on PATH. -- Reinstall browsers: `npx playwright install chromium` +**Playwright fails (missing browsers)** +- Install browsers: `npx playwright install chromium` **E2E fails with “test harness failed to load”** -- Check console error in the CI log (module import/exports mismatch). -- Verify all referenced JS files exist and export the expected names. +- Check the console error (module import/exports mismatch is the most common cause). +- Verify all referenced JS modules exist and export expected names. diff --git a/tests/test_diagnostics_flags.py b/tests/test_diagnostics_flags.py index 102d19e..4a289d0 100644 --- a/tests/test_diagnostics_flags.py +++ b/tests/test_diagnostics_flags.py @@ -1,6 +1,7 @@ """ Unit tests for Diagnostics Flags (R46). """ + import logging import unittest from unittest.mock import MagicMock, patch @@ -19,17 +20,21 @@ class TestDiagnosticsFlags(unittest.TestCase): def test_glob_matching(self): """Test glob pattern matching logic.""" - with patch.dict("os.environ", {"OPENCLAW_DIAGNOSTICS": "webhook.*, templates.*"}): + with patch.dict( + "os.environ", {"OPENCLAW_DIAGNOSTICS": "webhook.*, templates.*"} + ): mgr = DiagnosticsManager() - + # Direct matches self.assertTrue(mgr.is_enabled("webhook.submit")) self.assertTrue(mgr.is_enabled("webhook.validate")) self.assertTrue(mgr.is_enabled("templates.render")) - + # Non-matches self.assertFalse(mgr.is_enabled("llm.client")) - self.assertFalse(mgr.is_enabled("webhook")) # "webhook.*" matches "webhook.something", typically not "webhook" unless pattern is "webhook*" + self.assertFalse( + mgr.is_enabled("webhook") + ) # "webhook.*" matches "webhook.something", typically not "webhook" unless pattern is "webhook*" def test_empty_config(self): """Test default safe state.""" @@ -47,18 +52,18 @@ class TestDiagnosticsFlags(unittest.TestCase): mgr = DiagnosticsManager() # Mock enabled for "test" mgr.is_enabled = MagicMock(return_value=True) - + mock_logger = MagicMock() scoped = ScopedLogger(mock_logger, "test", mgr) - + sensitive_data = {"api_key": "sk-123456", "safe": "value"} scoped.debug("Test message", data=sensitive_data) - + # Verify call args mock_logger.info.assert_called_once() args, _ = mock_logger.info.call_args log_msg = args[0] - + self.assertIn("[DIAG:test]", log_msg) self.assertIn("***REDACTED***", log_msg) self.assertNotIn("sk-123456", log_msg) @@ -68,12 +73,13 @@ class TestDiagnosticsFlags(unittest.TestCase): """Test that disabled logger does nothing.""" mgr = DiagnosticsManager() mgr.is_enabled = MagicMock(return_value=False) - + mock_logger = MagicMock() scoped = ScopedLogger(mock_logger, "test", mgr) - + scoped.debug("Should not log") mock_logger.info.assert_not_called() + if __name__ == "__main__": unittest.main()