diff --git a/TODO.md b/TODO.md index 09f3c11..469125a 100644 --- a/TODO.md +++ b/TODO.md @@ -211,7 +211,7 @@ - [x] **`bidirectional_manager.py` state fields are never mutated** _(done 2026-07-23)_ — fixed by `run_loop_cycle()` in item #6 above; `stats`, `evolution_history`, `is_running`, and `last_check_time` are now all mutated each cycle. - [x] **`version_manager.py` registry file has no atomic write** — `_save_registry()` (lines 70-77, PR #72 branch) writes directly via `open(...,"w")` + `json.dump`; a crash/kill mid-write leaves a truncated file, and `_load_registry()` silently resets to an empty registry on parse failure, losing all version history - [ ] **`version_manager.py` has no locking around concurrent registry mutation** — overlapping `register_version`/`deploy_version` calls can interleave writes to `self.registry["versions"]`, risking lost updates or an inconsistent `current_version` -- [ ] **`model_fine_tuner.py:160-178` uses `trust_remote_code=True`** on both `AutoTokenizer`/`AutoModelForCausalLM.from_pretrained`, combined with a fallback (`_resolve_hf_base_model()`, lines 107-120) that uses `model_name` verbatim as an HF repo id for unknown families — a bad config value or env var (`EVOSEAL_CODER_MODEL`) can execute arbitrary remote code locally. `# nosec B615` suppresses the linter, not the risk +- [x] **`model_fine_tuner.py:160-178` uses `trust_remote_code=True`** _(done 2026-07-31)_ — changed both `AutoTokenizer` and `AutoModelForCausalLM.from_pretrained` calls to `trust_remote_code=False` (the safe default). The `# nosec B615` suppression comments remain because B615 flags missing `revision=`, not `trust_remote_code`; revision pinning is left to the caller. - [x] **`provider_manager.py` treats unhealthy providers as healthy when called from a running event loop** _(done 2026-07-28)_ — `get_best_available_provider()` (lines 100-111) and `list_providers()` (lines 196-201) create a health-check task via `loop.create_task(...)` but never await/consume it, then unconditionally set `is_healthy = True`, discarding the real result; the orphaned task can also produce unretrieved-exception warnings - [x] **`agentic_system.py:28` logger bypasses the logging handler hierarchy** _(done 2026-07-26)_ — `Logger("AgenticSystem")` was instantiated directly instead of via `logging.getLogger(name)`; `self.parent` stayed `None`, no handlers attached, and all INFO-level agent-orchestration logs were silently dropped. Changed to `get_logger("AgenticSystem")` to participate in the handler hierarchy - [x] **`agentic_workflow_agent.py:14` couples to a private API and can crash inside a running event loop** _(done 2026-07-28)_ — added public `WorkflowEngine.execute_step()` that detects a running event loop and offloads to a thread instead of re-entering `asyncio.run()`; `WorkflowAgent.act()` now calls the public method. Also added `act_async()` for callers already in an async context. 10 new unit tests cover: sync context, running-loop context, error propagation, missing component, and agent state tracking @@ -306,9 +306,9 @@ |----------|-------|------|-------| | 🔴 P0 | 11 | 11 | Original 5 complete; all 6 critical bugs from 2026-07-22 whole-repo review fixed (PRs #74, #76-#79) | | 🟠 P1 | 24 | 14 | Original safety/integration items done; +12 high-priority bugs from 2026-07-22 review; signal-handler init fix; safety.yaml created | -| 🟡 P2 | 30 | 21 | Co-evolution loop gaps (8 items, 8 done) + existing P2 + 13 medium bugs from 2026-07-22 review + 4 latent collect->train bugs found closing the loop (1 fixed, 1 new HF-format gap resolved); provider_manager health-check await fix; workflow-agent private-API/event-loop fix; checkpoint save/restore test | +| 🟡 P2 | 30 | 22 | Co-evolution loop gaps (8 items, 8 done) + existing P2 + 13 medium bugs from 2026-07-22 review + 4 latent collect->train bugs found closing the loop (1 fixed, 1 new HF-format gap resolved); provider_manager health-check await fix; workflow-agent private-API/event-loop fix; checkpoint save/restore test; trust_remote_code security fix | | 🟢 P3 | 24 | 15 | Makefile, pre-commit, Docker, ADRs, ADR refresh, CHANGELOG complete; +11 hygiene items from 2026-07-22 review; Ollama provider retry/backoff fix; local_models TTL cache | -| **Total** | **89** | **61** | | +| **Total** | **89** | **62** | | > Update this table as you complete items. Recommended flow: P0 → P1 → P2 → P3. > diff --git a/evoseal/fine_tuning/model_fine_tuner.py b/evoseal/fine_tuning/model_fine_tuner.py index 61363d1..541edbc 100644 --- a/evoseal/fine_tuning/model_fine_tuner.py +++ b/evoseal/fine_tuning/model_fine_tuner.py @@ -158,7 +158,7 @@ async def initialize_model(self) -> bool: # Load tokenizer. Base model is operator-configured; revision pinning is # left to the caller via base_model_path. self.tokenizer = AutoTokenizer.from_pretrained( # nosec B615 - base_model, trust_remote_code=True, padding_side="right" + base_model, trust_remote_code=False, padding_side="right" ) # Add pad token if missing @@ -167,15 +167,13 @@ async def initialize_model(self) -> bool: # Load model model_kwargs = { - "trust_remote_code": True, + "trust_remote_code": False, "torch_dtype": (torch.float16 if torch.cuda.is_available() else torch.float32), "device_map": "auto" if torch.cuda.is_available() else None, } # Revision pinning is left to the caller (see note above). - self.model = AutoModelForCausalLM.from_pretrained( # nosec B615 - base_model, **model_kwargs - ) + self.model = AutoModelForCausalLM.from_pretrained(base_model, **model_kwargs) # nosec B615 # Configure for training self.model.config.use_cache = False diff --git a/tests/unit/fine_tuning/test_model_fine_tuner.py b/tests/unit/fine_tuning/test_model_fine_tuner.py index 1c0ae55..b1227b0 100644 --- a/tests/unit/fine_tuning/test_model_fine_tuner.py +++ b/tests/unit/fine_tuning/test_model_fine_tuner.py @@ -1,15 +1,13 @@ -"""Tests for ModelFineTuner._tokenize_if_needed. +"""Tests for ModelFineTuner. -Covers the format-mismatch gap: TrainingDataBuilder's HuggingFace export -writes raw instruction/input/output columns, but Trainer + DataCollatorFor- -LanguageModeling require pre-tokenized input_ids. fine_tune_model() must -tokenize on load when the dataset isn't already tokenized, and leave an -already-tokenized dataset untouched. +Covers the format-mismatch gap (tokenization) and security defaults +(trust_remote_code must be False to prevent arbitrary code execution +from untrusted model repos). """ from __future__ import annotations -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch import pytest from datasets import Dataset @@ -60,3 +58,150 @@ def test_passes_max_length_through_to_tokenizer(self, fine_tuner): _, kwargs = fine_tuner.tokenizer.call_args assert kwargs["max_length"] == 128 + + +class TestTrustRemoteCodeDefault: + """Verify trust_remote_code defaults to False in initialize_model. + + trust_remote_code=True allows a HuggingFace repo to execute arbitrary + Python code on load — a remote-code-execution vector when combined + with the _resolve_hf_base_model() fallback that uses model_name + verbatim as an HF repo id for unknown families. + """ + + @pytest.mark.asyncio + async def test_from_pretrained_calls_use_trust_remote_code_false(self, tmp_path): + """Both from_pretrained calls receive trust_remote_code=False. + + Mocks the HF loading calls and asserts the kwarg on each, + consistent with the style of test_passes_max_length_through_to_tokenizer. + """ + import evoseal.fine_tuning.model_fine_tuner as mod + + tuner = ModelFineTuner.__new__(ModelFineTuner) + tuner.model_name = "deepseek-coder" + tuner.base_model_path = "deepseek-ai/deepseek-coder-6.7b-instruct" + tuner.output_dir = tmp_path + tuner.use_lora = True + tuner.use_qlora = False + tuner.tokenizer = None + tuner.model = None + tuner.peft_model = None + tuner.is_initialized = False + tuner.current_training = None + + mock_tokenizer = MagicMock() + mock_tokenizer.pad_token = None + mock_tokenizer.eos_token = "" + mock_model = MagicMock() + mock_model.config = MagicMock() + + # AutoTokenizer / AutoModelForCausalLM aren't in the module namespace + # because peft isn't installed (the try/except bailed early). Inject + # mock classes so the function can reference them, then patch + # from_pretrained on each. + mock_tok_cls = MagicMock() + mock_tok_cls.from_pretrained.return_value = mock_tokenizer + mock_mdl_cls = MagicMock() + mock_mdl_cls.from_pretrained.return_value = mock_model + + with ( + patch.object(mod, "TRANSFORMERS_AVAILABLE", True), + patch.object(mod, "AutoTokenizer", mock_tok_cls, create=True), + patch.object(mod, "AutoModelForCausalLM", mock_mdl_cls, create=True), + patch("torch.cuda.is_available", return_value=False), + ): + result = await tuner.initialize_model() + + assert result is True + + # Tokenizer call must pass trust_remote_code=False + _, tok_kwargs = mock_tok_cls.from_pretrained.call_args + assert tok_kwargs["trust_remote_code"] is False, ( + "AutoTokenizer.from_pretrained called with trust_remote_code!=False" + ) + + # Model call must pass trust_remote_code=False + _, mdl_kwargs = mock_mdl_cls.from_pretrained.call_args + assert mdl_kwargs["trust_remote_code"] is False, ( + "AutoModelForCausalLM.from_pretrained called with trust_remote_code!=False" + ) + + @pytest.mark.asyncio + async def test_initialize_returns_false_without_transformers(self, monkeypatch, tmp_path): + """When TRANSFORMERS_AVAILABLE is False, initialize_model returns False.""" + import evoseal.fine_tuning.model_fine_tuner as mod + + monkeypatch.setattr(mod, "TRANSFORMERS_AVAILABLE", False) + + tuner = ModelFineTuner.__new__(ModelFineTuner) + tuner.model_name = "test-model" + tuner.base_model_path = None + tuner.output_dir = tmp_path + tuner.use_lora = True + tuner.use_qlora = False + tuner.tokenizer = None + tuner.model = None + tuner.peft_model = None + tuner.is_initialized = False + tuner.current_training = None + + result = await tuner.initialize_model() + + assert result is False + assert tuner.is_initialized is False + + @pytest.mark.asyncio + async def test_initialize_returns_false_when_model_needs_trust_remote_code( + self, tmp_path, caplog + ): + """When a model requires trust_remote_code=True but gets False, initialize_model + catches the resulting exception, logs it, and returns False. + + This exercises the failure path that the reviewer flagged: a model that + used to load with trust_remote_code=True now hard-fails because custom + code execution is blocked. + """ + import logging + + import evoseal.fine_tuning.model_fine_tuner as mod + + tuner = ModelFineTuner.__new__(ModelFineTuner) + tuner.model_name = "custom-model" + tuner.base_model_path = "some-org/custom-model-needs-trust" + tuner.output_dir = tmp_path + tuner.use_lora = True + tuner.use_qlora = False + tuner.tokenizer = None + tuner.model = None + tuner.peft_model = None + tuner.is_initialized = False + tuner.current_training = None + + mock_tok_cls = MagicMock() + # Simulate the failure: a model that ships custom code raises when + # trust_remote_code=False. OSError is what transformers actually raises + # when the auto-map can't resolve the architecture without custom code. + mock_tok_cls.from_pretrained.side_effect = OSError( + "does not appear to have a file named configuration_auto.py. " + "Checkout 'https://huggingface.co/some-org/custom-model-needs-trust'" + ) + mock_mdl_cls = MagicMock() + + with ( + patch.object(mod, "TRANSFORMERS_AVAILABLE", True), + patch.object(mod, "AutoTokenizer", mock_tok_cls, create=True), + patch.object(mod, "AutoModelForCausalLM", mock_mdl_cls, create=True), + caplog.at_level(logging.ERROR, logger=mod.logger.name), + ): + result = await tuner.initialize_model() + + # Must surface the failure, not silently succeed + assert result is False + assert tuner.is_initialized is False + + # Model load should never have been attempted (tokenizer failed first) + mock_mdl_cls.from_pretrained.assert_not_called() + + # Error must be logged so operators can diagnose the root cause + assert "Error initializing model" in caplog.text