feat: add MTP draft model file support#118
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 12 minutes and 24 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request implements end-to-end support for MTP draft model files as an auxiliary file type. It adds a new ChangesMTP Draft Model File Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tama-web/src/components/pull_quant_wizard.rs (1)
233-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset auxiliary selections when starting a new repo search
At Line 362,
mtp_filenamesis built from persistent state, but in the repo-search path onlyselected_filenamesis cleared (Line 235).selected_mmproj_filenamesandselected_mtp_filenamescan leak stale values from a previous repo into the new/tama/v1/pullspayload.Suggested fix
on_search=Callback::new(move |rid| { error_msg.set(None); selected_filenames.set(std::collections::HashSet::new()); + selected_mmproj_filenames.set(std::collections::HashSet::new()); + selected_mtp_filenames.set(std::collections::HashSet::new()); gguf_context_length.set(None); model_id.set(None); context_settings.set(ContextSettings::default()); hf_metadata.set(HfModelMetadata::default()); available_quants.set(Vec::new()); + available_mmprojs.set(Vec::new()); + available_mtps.set(Vec::new());Also applies to: 357-365
🤖 Prompt for 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. In `@crates/tama-web/src/components/pull_quant_wizard.rs` around lines 233 - 241, In the on_search callback inside the PullQuantWizard component you currently clear selected_filenames but leave selected_mmproj_filenames and selected_mtp_filenames set, which lets stale selections leak into the /tama/v1/pulls payload; update the on_search handler (the closure where error_msg, selected_filenames, gguf_context_length, etc. are reset) to also clear selected_mmproj_filenames and selected_mtp_filenames (e.g., set them to new empty HashSet or Vec as appropriate), and apply the same change to the other repo-search start path noted in the review (the similar handler around lines 357-365) so both places reset those auxiliary selection states.
🧹 Nitpick comments (3)
crates/tama-core/src/db/migrations/migrations_tests.rs (1)
925-971: ⚡ Quick winStrengthen v28 regression assertions for claimed behaviors.
This test currently validates column existence and simple round-trip, but it does not assert the two behaviors called out in its own docs: NOCASE behavior and legacy-row NULL semantics after migrating from pre-v28.
Proposed test hardening
fn test_migration_v28_adds_selected_mtp_model_column() { let conn = Connection::open_in_memory().unwrap(); - run(&conn).unwrap(); + run_up_to(&conn, 27).unwrap(); + conn.execute( + "INSERT INTO model_configs (repo_id, backend) VALUES ('test/pre-v28', 'llama_cpp')", + [], + ) + .unwrap(); + run_up_to(&conn, 28).unwrap(); @@ - let _null_count: i64 = conn + let null_count: i64 = conn .query_row( "SELECT COUNT(*) FROM model_configs WHERE selected_mtp_model IS NULL", [], |row| row.get(0), ) .unwrap(); - // _null_count is informational; we don't fail on the count, but we want to - // at least exercise the query path. + assert_eq!(null_count, 1, "pre-v28 rows should have NULL selected_mtp_model"); @@ assert_eq!(stored.as_deref(), Some("mtp-F16.gguf")); + + let nocase_match: i64 = conn + .query_row( + "SELECT COUNT(*) FROM model_configs WHERE selected_mtp_model = 'MTP-F16.GGUF'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(nocase_match, 1, "selected_mtp_model comparisons should be case-insensitive"); }🤖 Prompt for 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. In `@crates/tama-core/src/db/migrations/migrations_tests.rs` around lines 925 - 971, The test test_migration_v28_adds_selected_mtp_model_column only checks column existence and a simple round-trip; update it to also assert (1) legacy-row NULL semantics by creating a pre-v28 model_configs row (create the pre-migration schema, insert a row without selected_mtp_model) before calling run(&conn), then run(&conn) and assert that selected_mtp_model IS NULL for that legacy row, and (2) COLLATE NOCASE behavior by inserting a row after migration with selected_mtp_model set to a value in one case (e.g., 'mtp-F16.gguf') and then verifying the same row is found or matched when queried/compared using a different case (e.g., query WHERE selected_mtp_model = 'MTP-F16.GGUF' or perform an upsert/insert-and-handle-conflict that would only collide if NOCASE is in effect) to prove case-insensitive comparisons; make these assertions inside test_migration_v28_adds_selected_mtp_model_column using the model_configs table and selected_mtp_model column names.crates/tama-web/src/api/models/crud/mod.rs (1)
125-126: ⚡ Quick winAdd focused tests for
mtp_modelmerge behavior inapply_model_body.Line 125 introduces merge semantics for
mtp_model, but there’s no direct assertion covering preserve-on-omit and update-on-provide behavior. Please add targeted tests for these cases.🤖 Prompt for 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. In `@crates/tama-web/src/api/models/crud/mod.rs` around lines 125 - 126, Add focused unit tests for apply_model_body that assert mtp_model is preserved when body.mtp_model is None and replaced when body.mtp_model is Some(...). Specifically, create two tests: one where base has mtp_model = Some(value) and body has mtp_model = None and assert the result.mtp_model == base.mtp_model (preserve-on-omit), and another where base has mtp_model = Some(old) and body has mtp_model = Some(new) and assert result.mtp_model == Some(new) (update-on-provide). Use the same model types/constructors already used in existing tests around apply_model_body to build base and body, call apply_model_body(base, body), and assert only mtp_model semantics (other fields may be defaulted or checked minimally).crates/tama-web/src/types/config.rs (1)
35-36: ⚡ Quick winAdd a serialization round-trip assertion for
QuantKind::Mtp.Line 35 introduces a new wire value (
"mtp"), buttest_quant_kind_serialization(Line 764+) still only assertsModelandMmproj. Please add aMtpround-trip assertion to lock the serde contract.Suggested test addition
#[test] fn test_quant_kind_serialization() { @@ let json_mmproj = serde_json::to_string(&QuantKind::Mmproj).unwrap(); assert!(json_mmproj.contains("mmproj")); let deserialized: QuantKind = serde_json::from_str(&json_mmproj).unwrap(); assert_eq!(deserialized, QuantKind::Mmproj); + + let json_mtp = serde_json::to_string(&QuantKind::Mtp).unwrap(); + assert!(json_mtp.contains("mtp")); + let deserialized: QuantKind = serde_json::from_str(&json_mtp).unwrap(); + assert_eq!(deserialized, QuantKind::Mtp); }🤖 Prompt for 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. In `@crates/tama-web/src/types/config.rs` around lines 35 - 36, The test test_quant_kind_serialization needs to include a round-trip assertion for the new QuantKind::Mtp variant: update the test to serialize "mtp" to QuantKind::Mtp and then deserialize back (or assert serde round-trip equality) just like the existing assertions for Model and Mmproj so the serde contract for the new wire value is locked; modify the test_quant_kind_serialization function to add an assertion that "mtp" <-> QuantKind::Mtp round-trips successfully.
🤖 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 `@crates/tama-core/src/config/resolve/mod.rs`:
- Around line 306-347: The code injects the llama.cpp-only "--spec-draft-model"
flag without checking backend capability; change the block that uses
server.model and server.mtp_model so it first checks the backend supports this
flag (e.g. call a capability check like self.backend.supports_spec_draft_model()
or add a method supports_spec_draft_model() and use it) and only then call
self.models_dir(), repo_path(...) and push the formatted "--spec-draft-model"
into grouped; ensure the existing checks (mtp kind == QuantKind::Mtp, existence
in server.quants, and duplicate flag detection via crate::config::flag_name)
remain unchanged but are nested under the new backend-capability guard so
non-llama backends never compute paths or inject the flag.
In `@crates/tama-core/src/proxy/tama_handlers/pull/download.rs`:
- Around line 476-502: Summary: docs/migration mismatch: migration
_0008_rebuild_model_tables.rs documents model_files.kind only allowing 'model'
or 'mmproj' but code writes 'mtp' (QuantKind::Mtp). Fix: either update the
migration documentation/comments in _0008_rebuild_model_tables.rs to include
'mtp' as an allowed kind, or enforce allowed values by altering the schema
(e.g., add a CHECK constraint in the migration that created/altered model_files)
to include 'model','mmproj','mtp'; reference the symbols QuantKind::Mtp,
QuantKind::Mmproj, QuantKind::Model and the migrations
_0007_create_model_configs.rs and _0008_rebuild_model_tables.rs when making the
change so the docs/schema stay consistent with the code.
In `@crates/tama-web/src/api/models/crud/mod.rs`:
- Around line 40-42: The new mtp_model field is unvalidated; update
validate_model_body to mirror the existing checks for quant and mmproj: when
mtp_model.is_some(), validate its length (same min/max bounds used for
quant/mmproj) and shape (same pattern or allowed charset) and return the same
validation error on failure; ensure you reference the mtp_model field in the
same validation block/path as quant/mmproj so it is rejected consistently with
other selector fields.
In `@crates/tama-web/src/pages/model_editor/api.rs`:
- Line 130: The backend merge currently uses body.mtp_model.or(base.mtp_model)
which treats a missing/null mtp_model the same and prevents explicit clearing;
change the request DTO and merge logic so mtp_model is Option<Option<T>> (an
outer Option to detect presence and an inner Option to allow explicit null),
then replace body.mtp_model.or(base.mtp_model) with a match like match
body.mtp_model { Some(inner) => inner, None => base.mtp_model } so an explicit
JSON null clears the stored value; update the affected types/serialization and
any callers of the DTO to compile against the new Option<Option<...>> shape.
---
Outside diff comments:
In `@crates/tama-web/src/components/pull_quant_wizard.rs`:
- Around line 233-241: In the on_search callback inside the PullQuantWizard
component you currently clear selected_filenames but leave
selected_mmproj_filenames and selected_mtp_filenames set, which lets stale
selections leak into the /tama/v1/pulls payload; update the on_search handler
(the closure where error_msg, selected_filenames, gguf_context_length, etc. are
reset) to also clear selected_mmproj_filenames and selected_mtp_filenames (e.g.,
set them to new empty HashSet or Vec as appropriate), and apply the same change
to the other repo-search start path noted in the review (the similar handler
around lines 357-365) so both places reset those auxiliary selection states.
---
Nitpick comments:
In `@crates/tama-core/src/db/migrations/migrations_tests.rs`:
- Around line 925-971: The test
test_migration_v28_adds_selected_mtp_model_column only checks column existence
and a simple round-trip; update it to also assert (1) legacy-row NULL semantics
by creating a pre-v28 model_configs row (create the pre-migration schema, insert
a row without selected_mtp_model) before calling run(&conn), then run(&conn) and
assert that selected_mtp_model IS NULL for that legacy row, and (2) COLLATE
NOCASE behavior by inserting a row after migration with selected_mtp_model set
to a value in one case (e.g., 'mtp-F16.gguf') and then verifying the same row is
found or matched when queried/compared using a different case (e.g., query WHERE
selected_mtp_model = 'MTP-F16.GGUF' or perform an
upsert/insert-and-handle-conflict that would only collide if NOCASE is in
effect) to prove case-insensitive comparisons; make these assertions inside
test_migration_v28_adds_selected_mtp_model_column using the model_configs table
and selected_mtp_model column names.
In `@crates/tama-web/src/api/models/crud/mod.rs`:
- Around line 125-126: Add focused unit tests for apply_model_body that assert
mtp_model is preserved when body.mtp_model is None and replaced when
body.mtp_model is Some(...). Specifically, create two tests: one where base has
mtp_model = Some(value) and body has mtp_model = None and assert the
result.mtp_model == base.mtp_model (preserve-on-omit), and another where base
has mtp_model = Some(old) and body has mtp_model = Some(new) and assert
result.mtp_model == Some(new) (update-on-provide). Use the same model
types/constructors already used in existing tests around apply_model_body to
build base and body, call apply_model_body(base, body), and assert only
mtp_model semantics (other fields may be defaulted or checked minimally).
In `@crates/tama-web/src/types/config.rs`:
- Around line 35-36: The test test_quant_kind_serialization needs to include a
round-trip assertion for the new QuantKind::Mtp variant: update the test to
serialize "mtp" to QuantKind::Mtp and then deserialize back (or assert serde
round-trip equality) just like the existing assertions for Model and Mmproj so
the serde contract for the new wire value is locked; modify the
test_quant_kind_serialization function to add an assertion that "mtp" <->
QuantKind::Mtp round-trips successfully.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6f98bf9-83ce-4e8f-bcaf-65fc9766eee3
📒 Files selected for processing (31)
crates/tama-cli/src/commands/model/create.rscrates/tama-cli/src/commands/model/pull.rscrates/tama-cli/src/handlers/server/add.rscrates/tama-cli/tests/tests.rscrates/tama-core/src/config/resolve/mod.rscrates/tama-core/src/config/resolve/tests/spec_decoding/mtp.rscrates/tama-core/src/config/types.rscrates/tama-core/src/db/backfill/hf_metadata.rscrates/tama-core/src/db/backfill/initial_backfill.rscrates/tama-core/src/db/migrations.rscrates/tama-core/src/db/migrations/_0028_add_selected_mtp_model.rscrates/tama-core/src/db/migrations/migrations_tests.rscrates/tama-core/src/db/queries/model_config_queries.rscrates/tama-core/src/db/queries/tests.rscrates/tama-core/src/db/queries/types.rscrates/tama-core/src/models/manager_tests.rscrates/tama-core/src/proxy/tama_handlers/pull/download.rscrates/tama-core/src/proxy/tama_handlers/pull/handlers.rscrates/tama-core/src/proxy/tama_handlers/types.rscrates/tama-core/src/tests/mmproj_detection_test.rscrates/tama-web/src/api/models/crud/mod.rscrates/tama-web/src/components/pull_quant_wizard.rscrates/tama-web/src/components/pull_wizard/components/selection_step.rscrates/tama-web/src/components/pull_wizard/mod.rscrates/tama-web/src/pages/model_editor/api.rscrates/tama-web/src/pages/model_editor/mod.rscrates/tama-web/src/pages/model_editor/spec_decoding_form.rscrates/tama-web/src/pages/model_editor/types.rscrates/tama-web/src/types/config.rsdocs/plans/2026-06-13-mtp-draft-model-files.mddocs/plans/README.md
Comment on lines
+306
to
+347
| // Inject --spec-draft-model from model card, only if: | ||
| // 1. mtp_model is set | ||
| // 2. The referenced quant has kind = Mtp | ||
| // 3. draft-mtp is in spec_decoding.spec_types (user enabled it) | ||
| // No backend gate — mirrors --mmproj; silently ignored by non-llama.cpp | ||
| // backends if they don't recognise the flag. | ||
| if let (Some(ref model_id), Some(ref mtp_name)) = (&server.model, &server.mtp_model) { | ||
| let has_draft_mtp = server | ||
| .spec_decoding | ||
| .spec_types | ||
| .iter() | ||
| .any(|t| t == "draft-mtp"); | ||
| if has_draft_mtp { | ||
| if let Some(mtp_entry) = server.quants.get(mtp_name.as_str()) { | ||
| if mtp_entry.kind == crate::config::QuantKind::Mtp { | ||
| let models_dir = self.models_dir()?; | ||
| let mtp_path = repo_path(&models_dir, model_id).join(&mtp_entry.file); | ||
| let already_has_draft = grouped.iter().any(|e| { | ||
| matches!(crate::config::flag_name(e), Some("--spec-draft-model")) | ||
| }); | ||
| if !already_has_draft { | ||
| let path_str = mtp_path.to_string_lossy(); | ||
| let quoted = crate::config::quote_value(&path_str); | ||
| grouped.push(format!("--spec-draft-model {}", quoted)); | ||
| } | ||
| } else { | ||
| tracing::warn!( | ||
| "mtp_model '{}' for model '{}' has kind={:?}, expected Mtp", | ||
| mtp_name, | ||
| model_id, | ||
| mtp_entry.kind | ||
| ); | ||
| } | ||
| } else { | ||
| tracing::warn!( | ||
| "mtp_model '{}' not found in ModelConfig for model '{}'", | ||
| mtp_name, | ||
| model_id | ||
| ); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Guard --spec-draft-model injection by backend capability.
This block injects a llama.cpp-specific flag before backend gating. On non-llama backends, it can still resolve paths via self.models_dir()? and fail arg construction (or pass unsupported flags), which is a startup-risk path.
Suggested fix
- // Inject --spec-draft-model from model card, only if:
+ let is_llama_cpp_backend = backend_is_llama_cpp(&server.backend);
+
+ // Inject --spec-draft-model from model card, only if:
// 1. mtp_model is set
// 2. The referenced quant has kind = Mtp
// 3. draft-mtp is in spec_decoding.spec_types (user enabled it)
- // No backend gate — mirrors --mmproj; silently ignored by non-llama.cpp
- // backends if they don't recognise the flag.
- if let (Some(ref model_id), Some(ref mtp_name)) = (&server.model, &server.mtp_model) {
+ // 4. backend is llama.cpp-compatible
+ if is_llama_cpp_backend
+ && let (Some(ref model_id), Some(ref mtp_name)) = (&server.model, &server.mtp_model)
+ {
let has_draft_mtp = server
.spec_decoding
.spec_types
@@
- let is_llama_cpp_backend = backend_is_llama_cpp(&server.backend);
+ // already computed above🤖 Prompt for 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.
In `@crates/tama-core/src/config/resolve/mod.rs` around lines 306 - 347, The code
injects the llama.cpp-only "--spec-draft-model" flag without checking backend
capability; change the block that uses server.model and server.mtp_model so it
first checks the backend supports this flag (e.g. call a capability check like
self.backend.supports_spec_draft_model() or add a method
supports_spec_draft_model() and use it) and only then call self.models_dir(),
repo_path(...) and push the formatted "--spec-draft-model" into grouped; ensure
the existing checks (mtp kind == QuantKind::Mtp, existence in server.quants, and
duplicate flag detection via crate::config::flag_name) remain unchanged but are
nested under the new backend-capability guard so non-llama backends never
compute paths or inject the flag.
Comment on lines
+476
to
+502
| // Tag the model_files row with the file kind so downstream | ||
| // consumers can distinguish MTP draft models from regular | ||
| // GGUF quants. `upsert_file` does not currently accept a | ||
| // `kind` parameter, so we issue a follow-up UPDATE. Mirrors | ||
| // the `QuantKind::from_filename` logic used to drive the | ||
| // card's `kind` field. | ||
| let db_kind = match crate::config::QuantKind::from_filename(&filename_clone) { | ||
| crate::config::QuantKind::Model => "model", | ||
| crate::config::QuantKind::Mmproj => "mmproj", | ||
| crate::config::QuantKind::Mtp => "mtp", | ||
| }; | ||
| if db_kind != "model" { | ||
| if let Err(e) = mgr.conn().execute( | ||
| "UPDATE model_files SET kind = ?1 | ||
| WHERE model_id = ?2 AND filename = ?3", | ||
| rusqlite::params![db_kind, mid, filename_clone], | ||
| ) { | ||
| tracing::warn!( | ||
| job_id = %job_id_clone, | ||
| model_id = mid, | ||
| file = %filename_clone, | ||
| kind = db_kind, | ||
| error = %e, | ||
| "model_files kind UPDATE failed" | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for model_files schema definition and migrations that add kind column
rg -n "model_files" --type sql -C 5 || rg -n "CREATE TABLE.*model_files" -C 10
rg -n "kind.*TEXT" crates/tama-core/src/db/migrations/ -C 3
rg -n "model_files.*kind" -C 3Repository: danielcherubini/tama
Length of output: 21961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all places that query/insert/update `model_files.kind`
rg -n --hidden --no-ignore-vcs "model_files.*kind|kind.*FROM model_files|UPDATE model_files.*SET kind|INSERT INTO model_files.*kind|SELECT .*kind.*FROM model_files" crates/tama-core/src
rg -n --hidden --no-ignore-vcs "pragma_table_info\('model_files'\)|model_files.*ADD COLUMN kind" crates/tama-core/src/db/migrationsRepository: danielcherubini/tama
Length of output: 1357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also search for "mmproj" or "mtp" handling tied to model_files kind/card kind
rg -n --hidden --no-ignore-vcs "model_files|QuantKind|mmproj|mtp|kind = 'model'|kind.*= 'mmproj'" crates/tama-core/src/proxy crates/tama-core/src/db crates/tama-core/srcRepository: danielcherubini/tama
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the Rust record/queries that read/write model_files.kind
rg -n "struct ModelFileRecord|ModelFileRecord|model_files\.kind|kind.*model_files" crates/tama-core/src/db/queries -S
rg -n "kind" crates/tama-core/src/db/queries/model_queries.rs -S
rg -n "kind" crates/tama-core/src/db/queries/types.rs -S
# Inspect the migration that added the column and its inline comment
sed -n '1,120p' crates/tama-core/src/db/migrations/_0007_create_model_configs.rs
sed -n '1,120p' crates/tama-core/src/db/migrations/_0008_rebuild_model_tables.rsRepository: danielcherubini/tama
Length of output: 701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show ModelFileRecord definition (includes `kind`)
sed -n '1,120p' crates/tama-core/src/db/queries/types.rs
# Show get_model_files/get_all_model_files mappings
sed -n '1,240p' crates/tama-core/src/db/queries/model_queries.rs
# Show the migrations that add `model_files.kind`
sed -n '1,120p' crates/tama-core/src/db/migrations/_0007_create_model_configs.rs
sed -n '1,120p' crates/tama-core/src/db/migrations/_0008_rebuild_model_tables.rs
# Show migrations test that checks the column exists
sed -n '1,120p' crates/tama-core/src/db/migrations/migrations_tests.rsRepository: danielcherubini/tama
Length of output: 21669
model_files.kind column is present (so the UPDATE won’t fail on missing column).
model_files.kind was added in migration crates/tama-core/src/db/migrations/_0007_create_model_configs.rs (ALTER TABLE model_files ADD COLUMN kind TEXT NOT NULL DEFAULT 'model';) and the migrations test asserts it exists (SELECT ... pragma_table_info('model_files') WHERE name='kind').
Minor follow-up: crates/tama-core/src/db/migrations/_0008_rebuild_model_tables.rs documents kind as only 'model' or 'mmproj', but the code writes 'mtp'—update the comment/docs (or add a CHECK constraint if you want to enforce allowed values).
🤖 Prompt for 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.
In `@crates/tama-core/src/proxy/tama_handlers/pull/download.rs` around lines 476 -
502, Summary: docs/migration mismatch: migration _0008_rebuild_model_tables.rs
documents model_files.kind only allowing 'model' or 'mmproj' but code writes
'mtp' (QuantKind::Mtp). Fix: either update the migration documentation/comments
in _0008_rebuild_model_tables.rs to include 'mtp' as an allowed kind, or enforce
allowed values by altering the schema (e.g., add a CHECK constraint in the
migration that created/altered model_files) to include 'model','mmproj','mtp';
reference the symbols QuantKind::Mtp, QuantKind::Mmproj, QuantKind::Model and
the migrations _0007_create_model_configs.rs and _0008_rebuild_model_tables.rs
when making the change so the docs/schema stay consistent with the code.
Comment on lines
+40
to
42
| pub mtp_model: Option<String>, | ||
| #[serde(default)] | ||
| pub args: Vec<String>, |
Contributor
There was a problem hiding this comment.
Validate mtp_model input length like other selector fields.
Line 40 adds a new user-controlled selector, but validate_model_body has no mtp_model bound/shape checks (unlike quant/mmproj). This leaves the new field as an unbounded string input path.
Suggested validation patch
const MAX_QUANT: usize = 128;
const MAX_MMPROJ: usize = 128;
+const MAX_MTP_MODEL: usize = 128;
const MAX_API_NAME: usize = 128;
@@
if let Some(ref mmproj) = body.mmproj {
if !mmproj.is_empty() && mmproj.len() > MAX_MMPROJ {
return Err(format!("mmproj must be at most {MAX_MMPROJ} characters"));
}
}
+ if let Some(ref mtp_model) = body.mtp_model {
+ if !mtp_model.is_empty() && mtp_model.len() > MAX_MTP_MODEL {
+ return Err(format!(
+ "mtp_model must be at most {MAX_MTP_MODEL} characters"
+ ));
+ }
+ }🤖 Prompt for 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.
In `@crates/tama-web/src/api/models/crud/mod.rs` around lines 40 - 42, The new
mtp_model field is unvalidated; update validate_model_body to mirror the
existing checks for quant and mmproj: when mtp_model.is_some(), validate its
length (same min/max bounds used for quant/mmproj) and shape (same pattern or
allowed charset) and return the same validation error on failure; ensure you
reference the mtp_model field in the same validation block/path as quant/mmproj
so it is rejected consistently with other selector fields.
| "model": form.model, | ||
| "quant": form.quant, | ||
| "mmproj": form.mmproj, | ||
| "mtp_model": form.mtp_model, |
Contributor
There was a problem hiding this comment.
Clearing mtp_model may not persist due backend merge fallback
Line 130 correctly sends null when form.mtp_model is cleared, but the backend merge path shown in
crates/tama-web/src/api/models/crud/mod.rs (body.mtp_model.or(base.mtp_model)) will keep the old
value instead of clearing it. Downstream impact: UI clears (e.g., after quant delete) can be lost after
save, leaving stale selected_mtp_model persisted.
🤖 Prompt for 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.
In `@crates/tama-web/src/pages/model_editor/api.rs` at line 130, The backend merge
currently uses body.mtp_model.or(base.mtp_model) which treats a missing/null
mtp_model the same and prevents explicit clearing; change the request DTO and
merge logic so mtp_model is Option<Option<T>> (an outer Option to detect
presence and an inner Option to allow explicit null), then replace
body.mtp_model.or(base.mtp_model) with a match like match body.mtp_model {
Some(inner) => inner, None => base.mtp_model } so an explicit JSON null clears
the stored value; update the affected types/serialization and any callers of the
DTO to compile against the new Option<Option<...>> shape.
Owner
Author
|
Fixed — callback now clears and alongside , preventing stale selections from leaking into new repo searches. |
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
1 participant
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
Add MTP (
mtp-*.gguf) draft model files as a first-class auxiliary file type, selectable via the Spec Decoding section of the model editor.Changes
QuantKind::Mtpvariant,ModelConfig.mtp_modelfield, DB migration_0028addingselected_mtp_modelcolumnPullRequest.mtp_filenamessupport, pull handler validation chain, download side effects (auto-select, stub creation,kind=Mtptagging)--mtp-model <path>injected inbuild_full_args()whenmtp_modelset ANDdraft-mtpinspec_typesArchitecture
Mirrors the existing
mmprojpattern with one key difference:--mtp-modelinjection is gated ondraft-mtpbeing inspec_types(not always-on like--mmproj). The MTP file selector lives in the Spec Decoding section (not Quants section).Test plan
cargo test --workspace— all tests pass (874 total)cargo clippy --workspace -- -D warnings— cleancargo fmt --all— cleancargo build --release --workspace— release build succeeds--mtp-modelin resolved args--mtp-modelNOT in resolved argsPlan: 2026-06-13-mtp-draft-model-files.md
Summary by CodeRabbit
New Features
Database Updates