fix(models): reject remote-only options for local sources#145
Merged
Conversation
Make local ModelSource.path handling fail loudly for non-default download/cache-only options while preserving cancellation and optional local SHA-256 verification. Update API docs, README, website guide, changelog, and regression coverage for the local path semantics.
There was a problem hiding this comment.
Pull request overview
This PR tightens local ModelSource.path(...) handling so remote/download-only ModelLoadOptions are rejected instead of ignored, while preserving local cancellation and SHA-256 verification behavior.
Changes:
- Added local-source option validation in the IO download manager.
- Added regression tests for manager-level and engine-level rejection paths.
- Updated API docs, README, website guide, and changelog to document local-vs-remote option semantics.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/src/platform/io/model_download_manager_io.dart |
Rejects unsupported remote/cache options for local path sources. |
lib/src/core/models/model_load_options.dart |
Documents supported local path option semantics. |
lib/src/core/models/download/model_download_manager_base.dart |
Documents expected local-source behavior for managers. |
test/unit/platform/io/model_download_manager_io_test.dart |
Adds manager regression tests for rejection and cancellation. |
test/unit/core/engine/engine_test.dart |
Adds public engine regression coverage for local option rejection. |
README.md |
Documents local path option boundary. |
website/docs/guides/model-lifecycle.md |
Updates lifecycle guide with local path semantics. |
CHANGELOG.md |
Records the behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
==========================================
+ Coverage 78.22% 78.25% +0.02%
==========================================
Files 75 75
Lines 9703 9715 +12
==========================================
+ Hits 7590 7602 +12
Misses 2113 2113
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ModelLoadOptionsfor localModelSource.path(...)inputs with a typedLlamaUnsupportedExceptioninstead of silently ignoring them.LlamaEngine.loadModelSource(...)path.Closes #137
Production-readiness scope
ModelSource.path(...)sources with default options, cancellation, and optionalsha256verification.DefaultModelDownloadManager; remote URL/cache flows are unchanged.cachePolicy,cacheDirectory, bearer tokens, custom headers,resume: false, and custommaxRetriesbefore local metadata is returned.Intentionally deferred follow-ups
Test Plan
dart format --output=none --set-exit-if-changed lib/src/core/models/model_load_options.dart lib/src/core/models/download/model_download_manager_base.dart lib/src/platform/io/model_download_manager_io.dart test/unit/platform/io/model_download_manager_io_test.dart test/unit/core/engine/engine_test.dartdart analyzedart analyze lib/src/core/models/model_load_options.dart lib/src/core/models/download/model_download_manager_base.dart lib/src/platform/io/model_download_manager_io.dart test/unit/platform/io/model_download_manager_io_test.dart test/unit/core/engine/engine_test.dartdart test test/unit/platform/io/model_download_manager_io_test.dartdart test test/unit/core/engine/engine_test.dartdart test -p vm -j 1 --exclude-tags local-only --reporter compact(840 passed, 1 skipped)git diff --check origin/maindart analyze test/unit/core/engine/engine_test.dart lib/src/platform/io/model_download_manager_io.dart test/unit/platform/io/model_download_manager_io_test.dartdart test test/unit/core/engine/engine_test.dartdart test test/unit/platform/io/model_download_manager_io_test.dartefb2aa89ed49d293adcc04fcfe5741c4703903da: all required jobs passingReview Notes
LlamaEngine.loadModelSource(...)local remote-only option regression test.dart:ioimport; fixed by removing filesystem use from that test while keeping the public rejection coverage.