diff --git a/.githooks/pre-push b/.githooks/pre-push new file mode 100644 index 0000000..4128009 --- /dev/null +++ b/.githooks/pre-push @@ -0,0 +1,7 @@ +#!/usr/bin/env bash +set -euo pipefail + +ROOT_DIR="$(git rev-parse --show-toplevel)" +cd "$ROOT_DIR" + +bash scripts/pre_push_checks.sh diff --git a/scripts/pre_push_checks.sh b/scripts/pre_push_checks.sh new file mode 100644 index 0000000..5d3b63d --- /dev/null +++ b/scripts/pre_push_checks.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +set -euo pipefail + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$ROOT_DIR" + +echo "[pre-push] repo: $ROOT_DIR" + +# Force a repo-local pre-commit cache to avoid readonly $HOME cache issues +# (common on WSL / mixed shell / sandbox setups). +export PRE_COMMIT_HOME="${PRE_COMMIT_HOME:-$ROOT_DIR/.tmp/pre-commit}" +mkdir -p "$PRE_COMMIT_HOME" + +require_cmd() { + local cmd="$1" + if ! command -v "$cmd" >/dev/null 2>&1; then + echo "[pre-push] ERROR: missing command: $cmd" >&2 + exit 1 + fi +} + +require_cmd pre-commit +require_cmd npm + +# Ensure Node 18+ for Playwright/E2E. +# CI uses Node 20; local baseline is Node 18. +if [ -n "${NVM_DIR:-}" ] && [ -s "${NVM_DIR}/nvm.sh" ]; then + # shellcheck disable=SC1090 + . "${NVM_DIR}/nvm.sh" +elif [ -s "${HOME}/.nvm/nvm.sh" ]; then + # shellcheck disable=SC1091 + . "${HOME}/.nvm/nvm.sh" +fi + +if command -v nvm >/dev/null 2>&1; then + if [ -f ".nvmrc" ]; then + nvm use >/dev/null + else + nvm use 18 >/dev/null + fi +fi + +require_cmd node +NODE_MAJOR="$(node -p "process.versions.node.split('.')[0]")" +if [ "$NODE_MAJOR" -lt 18 ]; then + echo "[pre-push] ERROR: Node >=18 required, current=$(node -v)" >&2 + echo "[pre-push] Hint: install nvm and run 'nvm use 18'." >&2 + exit 1 +fi + +echo "[pre-push] Node version: $(node -v)" +echo "[pre-push] 1/3 detect-secrets" +pre-commit run detect-secrets --all-files + +echo "[pre-push] 2/3 pre-commit all hooks" +pre-commit run --all-files + +echo "[pre-push] 3/3 npm test (Playwright)" +npm test + +echo "[pre-push] PASS" diff --git a/services/llm_client.py b/services/llm_client.py index a57a012..eb4846f 100644 --- a/services/llm_client.py +++ b/services/llm_client.py @@ -144,6 +144,31 @@ class LLMClient: self.model = DEFAULT_MODEL_BY_PROVIDER.get(self.provider, "default") + # R23 (plugin wiring) + R57 (precedence compatibility): + # CRITICAL: keep model alias resolution in __init__. + # Some callers instantiate LLMClient and execute immediately without calling Settings save flow, + # and tests assert that "model.resolve" runs during initialization. + # Removing this block regresses alias behavior (e.g., gpt4 -> gpt-4) and breaks unit tests. + # CI guard: tests/test_llm_client_plugins.py::test_model_alias_resolution_on_init. + if PLUGINS_AVAILABLE and self.model: + try: + from .plugins.async_bridge import run_async_in_sync_context + + resolve_ctx = RequestContext( + provider=self.provider, + model=str(self.model), + trace_id="init", + ) + resolved_model = run_async_in_sync_context( + plugin_manager.execute_first( + "model.resolve", resolve_ctx, str(self.model) + ) + ) + if isinstance(resolved_model, str) and resolved_model.strip(): + self.model = resolved_model.strip() + except Exception as e: + logger.warning(f"Model alias resolution failed (non-fatal): {e}") + self.timeout = ( timeout if timeout is not None else eff_config.get("timeout_sec", 120) ) diff --git a/tests/TEST_SOP.md b/tests/TEST_SOP.md index dad2ef0..6ea0faa 100644 --- a/tests/TEST_SOP.md +++ b/tests/TEST_SOP.md @@ -11,6 +11,20 @@ This document defines the **mandatory test workflow** for this repo. Run it **be ## Required Pre-Push Workflow (Must Run) +### Optional automation (recommended) + +Enable the repository-managed Git pre-push hook once: + +```bash +git config core.hooksPath .githooks +``` + +Then every `git push` will run: + +```bash +bash scripts/pre_push_checks.sh +``` + 1) Detect Secrets (baseline-based) ```bash diff --git a/tests/repro_r53_apply.py b/tests/repro_r53_apply.py index dcc8b4c..5928163 100644 --- a/tests/repro_r53_apply.py +++ b/tests/repro_r53_apply.py @@ -1,9 +1,11 @@ - -import unittest import asyncio -from unittest.mock import patch, MagicMock +import json +import unittest +from unittest.mock import MagicMock, patch + from api.config import config_put_handler + class TestR53ApplyFeedbackRepro(unittest.TestCase): def setUp(self): self.loop = asyncio.new_event_loop() @@ -17,15 +19,17 @@ class TestR53ApplyFeedbackRepro(unittest.TestCase): @patch("api.config.require_admin_token") @patch("api.config.check_rate_limit") @patch("api.config.require_same_origin_if_no_token") - def test_put_returns_apply_metadata(self, mock_csrf, mock_rate, mock_auth, mock_get_config, mock_update): + def test_put_returns_apply_metadata( + self, mock_csrf, mock_rate, mock_auth, mock_get_config, mock_update + ): # Setup mocks mock_csrf.return_value = None mock_rate.return_value = True mock_auth.return_value = (True, None) - + # update_config succeeds mock_update.return_value = (True, []) - + # get_effective_config returns the new state mock_get_config.return_value = ({"provider": "openai"}, {"provider": "file"}) @@ -42,6 +46,6 @@ class TestR53ApplyFeedbackRepro(unittest.TestCase): # Assertion: R53 requires an 'apply' field in the PUT /config response. self.assertIn("apply", body, "R53 Failure: Response missing 'apply' metadata") -import json + if __name__ == "__main__": unittest.main() diff --git a/tests/repro_r57_precedence.py b/tests/repro_r57_precedence.py index a00037f..ba4a649 100644 --- a/tests/repro_r57_precedence.py +++ b/tests/repro_r57_precedence.py @@ -1,31 +1,36 @@ - import unittest -from unittest.mock import patch, MagicMock +from unittest.mock import MagicMock, patch + from services.llm_client import LLMClient + class TestR57PrecedenceRepro(unittest.TestCase): @patch("services.runtime_config.get_effective_config") def test_provider_model_contamination(self, mock_get_config): # Scenario: Config has Provider A and Model A mock_get_config.return_value = ( {"provider": "anthropic", "model": "claude-3-opus", "base_url": ""}, - {"provider": "file", "model": "file"} + {"provider": "file", "model": "file"}, ) # User requests Provider B explicitly (e.g. via "Test Connection" with overrides) # They do NOT specify a model (failed to select one, or just testing provider default) client = LLMClient(provider="openai") - # CURRENT BEHAVIOR (Expected Failure): + # CURRENT BEHAVIOR (Expected Failure): # The client picks up "claude-3-opus" from config because it's not None. # But "claude-3-opus" is invalid for "openai". - + print(f"DEBUG: Client Provider={client.provider}, Model={client.model}") # Assertion: We want the model to BE CLEAN (None or provider default), not the config's model. # If this fails, it means we reproduced the issue. - self.assertNotEqual(client.model, "claude-3-opus", - "R57 Failure: Client inherited incompatible model from config for a different provider") + self.assertNotEqual( + client.model, + "claude-3-opus", + "R57 Failure: Client inherited incompatible model from config for a different provider", + ) + if __name__ == "__main__": unittest.main()