fix: default trust_remote_code to False in model_fine_tuner - #114
Conversation
Changed both AutoTokenizer.from_pretrained and AutoModelForCausalLM.from_pretrained calls from trust_remote_code=True to trust_remote_code=False. The known HF base models (DeepSeek, Qwen, CodeLlama, Mistral) are standard architectures that do not require remote code execution. Removed the # nosec B615 suppression comments since the security flag is now safe by default. The _resolve_hf_base_model() fallback still uses model_name verbatim for unknown families, but without trust_remote_code=True it can no longer execute arbitrary code from an untrusted repo. Added 3 regression tests: source inspection for the safe default, and behavior check when TRANSFORMERS_AVAILABLE is False. Closes TODO.md item: model_fine_tuner.py:160-178
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe fine-tuning loader now disables remote code execution for tokenizer and model loading. Tests verify the setting and unavailable-Transformers behavior. TODO tracking marks the security item complete and updates completion counts. ChangesRemote-code execution control
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/fine_tuning/test_model_fine_tuner.py`:
- Around line 89-107: Update test_initialize_returns_false_without_transformers
to accept the monkeypatch fixture and set the module-level
TRANSFORMERS_AVAILABLE flag to False before calling tuner.initialize_model().
Patch the module that defines ModelFineTuner.initialize_model, preserving the
existing assertions and avoiding real model loading.
- Around line 73-87: Strengthen test_source_uses_trust_remote_code_false to mock
both AutoTokenizer.from_pretrained and AutoModelForCausalLM.from_pretrained,
invoke ModelFineTuner.initialize_model, and assert each call receives
trust_remote_code explicitly as False in its keyword arguments. Replace the
source-text checks with call-level assertions so both loading call sites are
covered.
- Around line 1-6: Update the module docstring for ModelFineTuner tests to state
only that trust_remote_code=False prevents loading custom code from untrusted
model repositories; do not imply broader artifact or serialization safety unless
the covered loader configuration also enforces it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 194b1898-af39-4e87-9a5e-ae0e6aa8d065
📒 Files selected for processing (3)
TODO.mdevoseal/fine_tuning/model_fine_tuner.pytests/unit/fine_tuning/test_model_fine_tuner.py
B615 flags missing revision= parameter in from_pretrained(), not trust_remote_code. The PR removed these suppressions along with the trust_remote_code=True fix, but revision pinning is intentionally left to the caller. Restore # nosec B615 on both loading calls and update the regression test accordingly.
- Fix TODO.md changelog: nosec B615 comments were relocated, not removed; B615 flags missing revision=, not trust_remote_code - Move tokenizer nosec B615 back to opening from_pretrained( line so bandit matches it to the correct line number - Replace fragile inspect.getsource test with mock-based test that asserts trust_remote_code=False on both from_pretrained call kwargs - Fix test_initialize_returns_false_without_transformers to actually patch TRANSFORMERS_AVAILABLE=False via monkeypatch - Update PR body to correct inaccurate claim about removing nosec comments
|
Addressed review feedback: 1. TODO.md changelog mismatch — Fixed. The entry incorrectly said "removed the 2. 3. Hardcoding 4. Fragile source-inspection test — Fixed. Replaced Verification: |
Addresses review feedback on PR #114: the existing tests only asserted the kwarg value on mocked from_pretrained calls but never exercised the failure path where a model that needs custom code raises on load. The new test mocks from_pretrained to raise OSError (what transformers actually raises when the auto-map can't resolve the architecture without custom code) and verifies that: - initialize_model() returns False - is_initialized stays False - Model load is not attempted (tokenizer fails first) - The error is logged at ERROR level for operator diagnosis
Review feedback responsePoint 1: Hardcoded
|
| Model | Architecture | Needs custom code? |
|---|---|---|
deepseek-ai/deepseek-coder-6.7b-instruct |
LlamaForCausalLM | No |
Qwen/Qwen2.5-Coder-7B-Instruct |
Qwen2ForCausalLM | No |
codellama/CodeLlama-7b-Instruct-hf |
LlamaForCausalLM | No |
mistralai/Mistral-7B-Instruct-v0.2 |
MistralForCausalLM | No |
(Confirmed via HF API — all tagged library_name: transformers with standard model_type.)
Adding an opt-in trust_remote_code parameter to __init__ would reintroduce the exact RCE vector this PR closes — any caller (or misconfigured env var) could flip it back to True. The _resolve_hf_base_model() unknown-family fallback (L116-120) that uses model_name verbatim as an HF repo id is the attack surface; trust_remote_code=False is the fix.
If a future model genuinely needs custom code, the right path is: add it to HF_BASE_MODEL_BY_FAMILY with a pinned revision, or the operator sets base_model_path to a local path they've already vetted. That's a separate feature, not a regression from this change.
Point 2: Tests don't exercise the failure path
Valid — addressed. Added test_initialize_returns_false_when_model_needs_trust_remote_code which:
- Mocks
AutoTokenizer.from_pretrainedto raiseOSError(what transformers actually raises when the auto-map can't resolve architecture without custom code) - Asserts
initialize_model()returnsFalseandis_initializedstaysFalse - Asserts model load was never attempted (tokenizer fails first)
- Asserts the error is logged at
ERRORlevel with"Error initializing model"for operator diagnosis
Point 3: How from_pretrained exceptions are surfaced
Already handled. initialize_model() wraps the entire body in try/except Exception (L186-188) which catches the error, logs it via logger.error(f"Error initializing model: {e}"), and returns False. The new failure-path test explicitly asserts this log message is emitted. The full method body is visible in the diff — there's no hidden code path that silently swallows the exception.
* docs: update workspace prompt files to reflect current state - EVOLUTION.md: mark architecture doc and sequence diagram items done, update maturity caveat to reflect closed co-evolution loop, mark config validation and checkpoint save/restore tests done - TODO.md: mark 'Adopt workspace prompt file conventions' done, update P3 (15→16) and total (61→62) tracking counts * docs: update maturity caveat date stamp and add PR references Address review feedback on PR #113: - Update caveat date from 2026-07-24 to 2026-07-30 to match PR merge date - Add references to PRs #73, #89 and TODO.md section so the co-evolution loop-closure claim is auditable from the doc itself * fix(docs): correct config-validation date and reconcile P1/P2 summary counts - EVOLUTION.md: fix config-validation test date from 2026-07-24 to 2026-07-30 (actual commit 85e92c2 was 2026-07-30, not 2026-07-24) - TODO.md: fix P1 done count from 14 to 13 (13 actual checked boxes) - TODO.md: fix P2 done count from 21 to 24 (24 actual checked boxes) - TODO.md: update total from 62 to 64 to match corrected per-priority counts * fix: default trust_remote_code to False in model_fine_tuner (#114) * fix: default trust_remote_code to False in model_fine_tuner Changed both AutoTokenizer.from_pretrained and AutoModelForCausalLM.from_pretrained calls from trust_remote_code=True to trust_remote_code=False. The known HF base models (DeepSeek, Qwen, CodeLlama, Mistral) are standard architectures that do not require remote code execution. Removed the # nosec B615 suppression comments since the security flag is now safe by default. The _resolve_hf_base_model() fallback still uses model_name verbatim for unknown families, but without trust_remote_code=True it can no longer execute arbitrary code from an untrusted repo. Added 3 regression tests: source inspection for the safe default, and behavior check when TRANSFORMERS_AVAILABLE is False. Closes TODO.md item: model_fine_tuner.py:160-178 * fix: restore nosec B615 for revision pinning B615 flags missing revision= parameter in from_pretrained(), not trust_remote_code. The PR removed these suppressions along with the trust_remote_code=True fix, but revision pinning is intentionally left to the caller. Restore # nosec B615 on both loading calls and update the regression test accordingly. * fix: address review feedback on trust_remote_code PR - Fix TODO.md changelog: nosec B615 comments were relocated, not removed; B615 flags missing revision=, not trust_remote_code - Move tokenizer nosec B615 back to opening from_pretrained( line so bandit matches it to the correct line number - Replace fragile inspect.getsource test with mock-based test that asserts trust_remote_code=False on both from_pretrained call kwargs - Fix test_initialize_returns_false_without_transformers to actually patch TRANSFORMERS_AVAILABLE=False via monkeypatch - Update PR body to correct inaccurate claim about removing nosec comments * test: add failure-path test for trust_remote_code=False incompatibility Addresses review feedback on PR #114: the existing tests only asserted the kwarg value on mocked from_pretrained calls but never exercised the failure path where a model that needs custom code raises on load. The new test mocks from_pretrained to raise OSError (what transformers actually raises when the auto-map can't resolve the architecture without custom code) and verifies that: - initialize_model() returns False - is_initialized stays False - Model load is not attempted (tokenizer fails first) - The error is logged at ERROR level for operator diagnosis
1 participant
What
Changed both
AutoTokenizer.from_pretrainedandAutoModelForCausalLM.from_pretrainedcalls fromtrust_remote_code=Truetotrust_remote_code=False(the safe default). The# nosec B615suppression comments remain because B615 flags missingrevision=, nottrust_remote_code— revision pinning is left to the caller.Why
trust_remote_code=Trueallows a HuggingFace repo to execute arbitrary Python code on model load. Combined with the_resolve_hf_base_model()fallback that usesmodel_nameverbatim as an HF repo id for unknown families, a bad config value or env var (EVOSEAL_CODER_MODEL) could execute arbitrary remote code locally.The known HF base models (DeepSeek, Qwen, CodeLlama, Mistral) are standard architectures that do not require remote code execution —
trust_remote_code=Falseis the correct default.Changes
model_fine_tuner.py:trust_remote_code=True→Falsein both loading callstest_model_fine_tuner.py: Added 2 regression tests — mock-based verification that bothfrom_pretrainedcalls receivetrust_remote_code=False, plus behavior check when TRANSFORMERS_AVAILABLE is FalseVerification
ruff format --check .✅ (400 files formatted)ruff check evoseal/ tests/✅ All checks passedpytest tests/unit/fine_tuning/test_model_fine_tuner.py -q→ 5 passed ✅Closes TODO.md item:
model_fine_tuner.py:160-178Summary by CodeRabbit
Security
Bug Fixes
Tests