fix(model-hf): reject rooted POSIX paths in delete on Windows too - #1854
Virgile-pct wants to merge 2 commits into
Conversation
`parse_delete_model_ref` rejects filesystem paths by testing `is_absolute()`
plus a few literal shapes. Windows does not consider a leading `/` absolute,
since that needs a drive letter or a UNC prefix, so a rooted POSIX path slipped
through the guard there and fell into the Hugging Face branch instead.
Measured on Windows 11 by calling `resolve_model_identifier` directly:
/tmp/model.gguf is_absolute=false -> "Expected a model stem or
Hugging Face ref..."
/home/user/model.gguf is_absolute=false -> same
C:\models\model.gguf is_absolute=true -> rejected as a path
C:/models/model.gguf is_absolute=true -> rejected as a path
\\server\share\model.gguf is_absolute=true -> rejected as a path
./model.gguf is_absolute=false -> rejected as a path
~/model.gguf is_absolute=false -> rejected as a path
So `mesh-llm models delete /tmp/model.gguf` answered with a message about model
stems and Hugging Face refs rather than saying that paths are not supported.
Wrong answer, not a dangerous one, and only on Windows.
A leading `/` is now rejected outright. No model stem or Hugging Face ref starts
with one on any platform, and on Unix `is_absolute()` already covered it, so
behaviour there is unchanged.
The test grew from one input to seven, covering the rooted POSIX, Windows drive,
UNC, relative and tilde shapes. `C:/models/model.gguf` is deliberately absent:
Linux has no reason to treat it as a path, and asserting it would pass here and
fail in CI.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe delete model reference parser now rejects POSIX-rooted inputs in addition to absolute paths. Tests cover Unix, Windows, UNC, relative, and tilde-based filesystem path shapes. ChangesDelete Path Validation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Delete-reference validation now rejects the additional rooted path forms, with matching coverage; no merge-blocking risk is identified. 🚥 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 |
i386
left a comment
There was a problem hiding this comment.
Reviewed the synchronized exact head. The Windows deletion guard covers leading-slash rooted paths and preserves valid relative paths; model-hf and full host-runtime package suites pass locally.
On Windows,
mesh-llm models delete /tmp/model.ggufanswers with a message about model stems and Hugging Face refs instead of saying that filesystem paths are not supported.Why
parse_delete_model_refrejects paths withis_absolute()plus a few literal shapes. Windows does not consider a leading/absolute, since that needs a drive letter or a UNC prefix, so a rooted POSIX path passes the guard and falls into the Hugging Face branch.Measured on Windows 11 by calling
resolve_model_identifierdirectly:is_absolute()/tmp/model.gguf/home/user/model.ggufC:\models\model.ggufC:/models/model.gguf\\server\share\model.gguf./model.gguf~/model.ggufWrong answer rather than a dangerous one, and only on Windows.
What this changes
A leading
/is rejected outright. No model stem or Hugging Face ref starts with one on any platform, and on Unixis_absolute()already covered it, so behaviour there is unchanged.The test grew from one input to the seven above, minus
C:/models/model.gguf, which is deliberately absent: Linux has no reason to treat it as a path, so asserting it would pass on my box and fail in CI.Measured effect, including a number that needs explaining
Full suite on Windows 11 against
mainat3f4f1c35a:mainThe count went up, so here is the whole of it. One test is fixed,
models::delete_tests::resolve_model_identifier_rejects_filesystem_paths, which is the one this branch targets. Two others failed that do not fail onmain:runtime::config_state::tests::sync::config_sync_state_apply_preserves_additive_defaults_sectionsruntime::config_state::tests::sync::config_sync_state_apply_preserves_nested_sections_and_updates_hashBoth pass 5 runs out of 5 in isolation.
runtime::config_statenever callsresolve_model_identifierorparse_delete_model_ref, and this branch touches one guard inmodel-hfplus its test. This box produces one to three intermittent failures per full run, varying between runs, and today alone it surfaced three different ones across unrelated modules.I would rather show you the raw number with that explanation than a tidier one.
Gates
cargo test -p model-hf: 47 passed, 0 failed.cargo test -p mesh-llm-host-runtime: as above.cargo fmt --all --check: clean.cargo clippy -p model-hf -p mesh-llm-host-runtime --all-targets: 8 lints, complete sorted set identical tomain.cargo run -p xtask -- repo-consistency no-console-print: passed.scripts/check-env-mutation-contract.pyandpython -m unittest scripts.tests.test_env_mutation_contract: passed.Summary by CodeRabbit