Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
>
Expand Down
8 changes: 3 additions & 5 deletions evoseal/fine_tuning/model_fine_tuner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
159 changes: 152 additions & 7 deletions tests/unit/fine_tuning/test_model_fine_tuner.py
Original file line number Diff line number Diff line change
@@ -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).
"""
Comment thread
coderabbitai[bot] marked this conversation as resolved.

from __future__ import annotations

from unittest.mock import MagicMock
from unittest.mock import MagicMock, patch

import pytest
from datasets import Dataset
Expand Down Expand Up @@ -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 = "</s>"
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
Comment thread
coderabbitai[bot] marked this conversation as resolved.

@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
Loading