fix(skippy): preserve native tool-call semantics - #1144
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR preserves omitted, Runtime generation and chat protocol
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
74d1231 to
5be9837
Compare
|
@michaelneale @ndizazzo we need to get this in and also push upstream |
5be9837 to
fc789c4
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/skippy-server/src/frontend/local_generation.rs`:
- Around line 680-682: Split the touched Rust files into semantically named
private modules, keeping each resulting file under 1,000 lines and moving
related tests with the extracted behavior: in
crates/skippy-server/src/frontend/local_generation.rs lines 680-682, extract a
generation phase such as proposal orchestration or prompt prefill; in
crates/skippy-server/src/runtime_state.rs lines 462-471, extract a runtime
responsibility such as lane lifecycle or frame operations; and in
crates/skippy-server/src/frontend/linear_proposal.rs lines 474-476, extract a
proposal responsibility such as repair or ingress handling. Preserve existing
behavior and update module visibility/imports as needed.
In
`@third_party/llama.cpp/patches/0050-Preserve-OpenAI-message-content-presence.patch`:
- Around line 74-100: Update the unchanged try block in
test_msgs_oaicompat_json_conversion so parsing a role-only assistant message no
longer expects an exception, matching the successful parse behavior added by the
new assertions. Remove or revise only that stale failure expectation while
preserving the surrounding validation.
🪄 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 Plus
Run ID: 06d8256b-e811-4bf6-9445-7d0a9a396470
📒 Files selected for processing (12)
crates/openai-frontend/src/chat.rscrates/skippy-runtime/src/activation.rscrates/skippy-runtime/src/session.rscrates/skippy-runtime/src/tests.rscrates/skippy-server/src/frontend/generation/parsing.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/tests/prompting.rscrates/skippy-server/src/runtime_state.rsthird_party/llama.cpp/patches/0048-Preserve-grammar-state-during-sampled-verification.patchthird_party/llama.cpp/patches/0049-Accept-tagged-tool-arguments-in-object-order.patchthird_party/llama.cpp/patches/0050-Preserve-OpenAI-message-content-presence.patch
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
this is good but need to get the build clean.
316fb0c to
3a9a525
Compare
3553d33 to
453a871
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
third_party/llama.cpp/patches/0065-Fix-role-only-OpenAI-chat-test-expectation.patch (1)
18-25: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a positive test for role-only messages.
The patch removes the old rejection test but does not verify the new contract. Add assertions that a message with omitted
contentparses successfully and round-trips without acontentfield. Also verify that omittedcontentremains distinct from explicit emptycontent.🤖 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 `@third_party/llama.cpp/patches/0065-Fix-role-only-OpenAI-chat-test-expectation.patch` around lines 18 - 25, Update the role-only chat test around common_chat_msgs_parse_oaicompat to assert that a message containing only "role" parses successfully and serializes back without a "content" field. Add a separate assertion for explicit empty content, ensuring it remains distinguishable from omitted content during the parse and round-trip checks.crates/skippy-server/src/frontend/local_generation/tests.rs (1)
51-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a behavioral guard over source-text matching.
This test asserts on the text of
token_generation.rs. It passes or fails on formatting rather than behavior. If rustfmt wraps the call, for exampleruntime\n .decode_sampled_mtp(, the.decode_sampled_mtp(substring no longer matches and the test fails without any behavior change. Theconcat!splits also obscure intent. Consider asserting the runtime call through a fake runtime or a counter, or keep the check and add a comment that explains the formatting coupling.🤖 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/skippy-server/src/frontend/local_generation/tests.rs` around lines 51 - 56, Replace the source-text assertions in local_native_mtp_decode_uses_non_frame_runtime_api with a behavioral test using a fake runtime or call counter that verifies decode_sampled_mtp is invoked and decode_frame_sampled_mtp is not. Avoid concat!-based formatting-sensitive matching; only retain text matching if no behavioral seam exists, documenting its formatting coupling.crates/skippy-server/src/frontend/linear_proposal/execution.rs (1)
263-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord lock timing for the checkpoint-retirement branch.
The retirement branch acquires the runtime lock and then returns
LinearProposalRepairTiming::default(). Sorepair_elapsed_usis0,runtime_lock_wait_usandruntime_lock_hold_usexclude this acquisition, andruntime_lock_acquirescounts one acquire instead of two.crates/skippy-server/src/frontend/local_generation/token_generation.rs(Lines 711-724) folds these receipt fields into the decode-phase lock metrics, so full-accept proposals under-report runtime lock pressure.♻️ Proposed fix to measure the retirement branch
if canonical_position >= position_after_verification { + let retire_timer = Instant::now(); + let retire_lock_timer = Instant::now(); let mut runtime = self.runtime.lock().map_err(|_| { OpenAiError::backend("runtime lock poisoned during verify retirement") })?; + let runtime_lock_wait_us = elapsed_us(retire_lock_timer); + let retire_hold_timer = Instant::now(); runtime .retire_verify_checkpoint(session_id, checkpoint_start, checkpoint_count as u64) .map_err(openai_backend_error)?; - return Ok(LinearProposalRepairTiming::default()); + return Ok(LinearProposalRepairTiming { + elapsed_us: elapsed_us(retire_timer), + runtime_lock_wait_us, + runtime_lock_hold_us: elapsed_us(retire_hold_timer), + runtime_lock_acquires: 1, + }); }🤖 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/skippy-server/src/frontend/linear_proposal/execution.rs` around lines 263 - 271, Update the retirement branch in the linear proposal execution flow to measure the runtime lock acquisition and hold durations, increment the lock-acquire count, and include the retirement work in repair_elapsed_us before returning. Populate LinearProposalRepairTiming with these measured values instead of returning default(), while preserving the existing lock-poison and retirement error handling.crates/skippy-server/src/frontend/linear_proposal.rs (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider importing
LinearProposalExecutionParamsfrom its owning module.
crates/skippy-server/src/frontend/local_generation/token_generation.rs(Lines 15-19) importsLinearProposalExecutionParamsthrough this parent re-export.elapsed_usneeds the re-export only iflinear_proposal.rsitself uses it. If the parent module does not use these items, let consumers importcrate::frontend::linear_proposal::execution::{...}and drop the re-export.Based on the coding guideline "Minimize crate-root re-exports. Temporary compatibility re-exports are allowed during refactors, but new code should import from the owning module directly and transitional re-exports should be removed afterward."
Source: Coding guidelines
🤖 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/skippy-server/src/frontend/linear_proposal.rs`:
- Around line 448-451: Configure the CI correctness-test job to set
SKIPPY_CORRECTNESS_MODEL so
batched_sampled_verification_matches_serial_across_lazy_grammar_trigger executes
instead of being skipped. Keep the chat_sampling_metadata admission logic
unchanged; ensure the configured model supports the grammar-equivalence test.
In `@crates/skippy-server/src/frontend/local_generation/token_generation.rs`:
- Around line 34-981: Refactor generate_local_tokens into semantically named
private phase helpers, separating prompt prefill, KV
lookup/restore/record/eviction, sampling or proposal setup, and the decode loop
including native MTP, hooks, and telemetry. Preserve the existing control flow,
error propagation, token callback behavior, receipt tracking, cache statistics,
and cleanup while reducing the size and responsibility of generate_local_tokens.
Use helpers such as prefill_prompt, restore_or_record_kv, and run_decode_loop
where they match the extracted phases.
---
Nitpick comments:
In `@crates/skippy-server/src/frontend/linear_proposal/execution.rs`:
- Around line 263-271: Update the retirement branch in the linear proposal
execution flow to measure the runtime lock acquisition and hold durations,
increment the lock-acquire count, and include the retirement work in
repair_elapsed_us before returning. Populate LinearProposalRepairTiming with
these measured values instead of returning default(), while preserving the
existing lock-poison and retirement error handling.
In `@crates/skippy-server/src/frontend/local_generation/tests.rs`:
- Around line 51-56: Replace the source-text assertions in
local_native_mtp_decode_uses_non_frame_runtime_api with a behavioral test using
a fake runtime or call counter that verifies decode_sampled_mtp is invoked and
decode_frame_sampled_mtp is not. Avoid concat!-based formatting-sensitive
matching; only retain text matching if no behavioral seam exists, documenting
its formatting coupling.
In
`@third_party/llama.cpp/patches/0065-Fix-role-only-OpenAI-chat-test-expectation.patch`:
- Around line 18-25: Update the role-only chat test around
common_chat_msgs_parse_oaicompat to assert that a message containing only "role"
parses successfully and serializes back without a "content" field. Add a
separate assertion for explicit empty content, ensuring it remains
distinguishable from omitted content during the parse and round-trip checks.
🪄 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 Plus
Run ID: 05041992-df4a-4fb0-a6e4-8aaf4d40cbc1
📒 Files selected for processing (18)
crates/openai-frontend/src/chat.rscrates/skippy-runtime/src/activation.rscrates/skippy-runtime/src/session.rscrates/skippy-runtime/src/tests.rscrates/skippy-server/src/frontend/generation/parsing.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/linear_proposal/execution.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/local_generation/tests.rscrates/skippy-server/src/frontend/local_generation/token_generation.rscrates/skippy-server/src/frontend/tests/prompting.rscrates/skippy-server/src/runtime_state.rscrates/skippy-server/src/runtime_state/frame_operations.rscrates/skippy-server/src/runtime_state/lane_lifecycle.rsthird_party/llama.cpp/patches/0048-Preserve-grammar-state-during-sampled-verification.patchthird_party/llama.cpp/patches/0049-Accept-tagged-tool-arguments-in-object-order.patchthird_party/llama.cpp/patches/0050-Preserve-OpenAI-message-content-presence.patchthird_party/llama.cpp/patches/0065-Fix-role-only-OpenAI-chat-test-expectation.patch
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/skippy-runtime/src/session.rs
- crates/skippy-runtime/src/tests.rs
- third_party/llama.cpp/patches/0048-Preserve-grammar-state-during-sampled-verification.patch
- crates/skippy-server/src/frontend/tests/prompting.rs
- crates/skippy-runtime/src/activation.rs
- crates/skippy-server/src/frontend/generation/parsing.rs
- crates/openai-frontend/src/chat.rs
- third_party/llama.cpp/patches/0050-Preserve-OpenAI-message-content-presence.patch
- third_party/llama.cpp/patches/0049-Accept-tagged-tool-arguments-in-object-order.patch
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/skippy-server/src/frontend/local_generation/token_generation.rs (2)
1210-1219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
#[allow(dead_code)]with a#[cfg(test)]gate on the frame method.
decode_frame_sampled_mtpexists only so thatnative_mtp_dispatch_counts_for_testcan assert that local decode never calls the frame API. Production code never calls it, so the trait method needs the allow attribute. Gate the method and itsRuntimeStateimplementation with#[cfg(test)]instead. This removes the suppression and keeps the negative test.As per coding guidelines, "Do not leave compiler or lint warnings in touched Rust code; fix warnings rather than using
#[allow(...)]unless there is a clear reason and explicit developer approval."♻️ Proposed change
- #[allow(dead_code)] + #[cfg(test)] fn decode_frame_sampled_mtp(Apply the same
#[cfg(test)]attribute to the matching method inimpl NativeMtpRuntime for RuntimeStateat Line 1233.🤖 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/skippy-server/src/frontend/local_generation/token_generation.rs` around lines 1210 - 1219, Replace #[allow(dead_code)] with #[cfg(test)] on the decode_frame_sampled_mtp trait method, and apply the same #[cfg(test)] gate to its matching RuntimeState implementation in impl NativeMtpRuntime. Keep the method available for native_mtp_dispatch_counts_for_test while excluding it from production builds.Source: Coding guidelines
245-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared prefill telemetry attributes into one helper.
Lines 245-276 and Lines 331-371 build the same attribute set:
prefill_token_count,prefill_chunk_count,skippy.kv.restored_prefill,skippy.kv.restored_prefill_tokens,skippy.kv.prefill_suffix_tokens,skippy.kv.recorded_pages, the three runtime-lock fields, and both session-stat blocks. Only the values differ. Extract a helper that takes those values and returns the attribute map. This keeps the two prefill paths in sync when an attribute is added.🤖 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/skippy-server/src/frontend/local_generation/token_generation.rs` around lines 245 - 276, Extract the duplicated prefill telemetry construction from the two prefill paths into a shared helper near the existing telemetry methods in the relevant implementation. Have the helper accept the differing token, chunk, restored-prefill, recorded-page, runtime-lock, and session-stat values and return the populated attribute map, then replace both inline blocks with calls to it while preserving each path’s values.
🤖 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 @.github/workflows/ci.yml:
- Around line 461-465: Pin and validate the Qwen3 correctness fixture in both
workflows: in .github/workflows/ci.yml lines 461-465 and
.github/workflows/pr_builds.yml lines 777-781, assign SKIPPY_CORRECTNESS_MODEL
to the downloaded model path, add the same fixed --revision to hf download, and
verify the expected SHA-256 immediately before cargo test -p skippy-runtime uses
it.
---
Nitpick comments:
In `@crates/skippy-server/src/frontend/local_generation/token_generation.rs`:
- Around line 1210-1219: Replace #[allow(dead_code)] with #[cfg(test)] on the
decode_frame_sampled_mtp trait method, and apply the same #[cfg(test)] gate to
its matching RuntimeState implementation in impl NativeMtpRuntime. Keep the
method available for native_mtp_dispatch_counts_for_test while excluding it from
production builds.
- Around line 245-276: Extract the duplicated prefill telemetry construction
from the two prefill paths into a shared helper near the existing telemetry
methods in the relevant implementation. Have the helper accept the differing
token, chunk, restored-prefill, recorded-page, runtime-lock, and session-stat
values and return the populated attribute map, then replace both inline blocks
with calls to it while preserving each path’s values.
🪄 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 Plus
Run ID: a783151a-60c0-446e-9114-cb62d5a9a965
📒 Files selected for processing (9)
.agents/skills/manage-ci/references/current-inventory.md.github/workflows/ci.yml.github/workflows/pr_builds.ymlci/ci.mdcrates/skippy-server/src/frontend/linear_proposal/execution.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/local_generation/tests.rscrates/skippy-server/src/frontend/local_generation/token_generation.rsthird_party/llama.cpp/patches/0065-Fix-role-only-OpenAI-chat-test-expectation.patch
🚧 Files skipped from review as they are similar to previous changes (3)
- third_party/llama.cpp/patches/0065-Fix-role-only-OpenAI-chat-test-expectation.patch
- crates/skippy-server/src/frontend/local_generation/tests.rs
- crates/skippy-server/src/frontend/linear_proposal/execution.rs
| hf download \ | ||
| jc-builds/Qwen3-0.6B-Q4_K_M-GGUF \ | ||
| Qwen3-0.6B-Q4_K_M.gguf \ | ||
| --local-dir "$RUNNER_TEMP/skippy-correctness-model" | ||
| test -s "$SKIPPY_CORRECTNESS_MODEL" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Skill and CI contract presence =="
git ls-files .agents/skills/manage-ci/SKILL.md ci/ci.md .github/workflows/ci.yml .github/workflows/pr_builds.yml
echo
echo "== Relevant workflow excerpts =="
for f in .github/workflows/ci.yml .github/workflows/pr_builds.yml; do
echo "--- $f ---"
nl -ba "$f" | sed -n '435,475p'
echo "--- $f ---"
nl -ba "$f" | sed -n '755,795p'
done
echo
echo "== Search for SKIPPY_CORRECTNESS_MODEL and model refs =="
rg -n 'SKIPPY_correctness|SKIPPY_CORRECTNESS|Qwen3|skippy-correctness-model|huggingface|hf download|--revision|sha256|sha-256|checksum' .github/workflows .agents ci . 2>/dev/null | head -200
echo
echo "== Check huggingface-cli availability/version and download cmd help snippets if present =="
command -v huggingface-cli || true
huggingface-cli download --help 2>&1 | rg 'revision|local-dir|ignore|cache|verify|sha' || trueRepository: Mesh-LLM/mesh-llm
Length of output: 400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate relevant files =="
for f in .agents/skills/manage-ci/SKILL.md ci/ci.md .github/workflows/ci.yml .github/workflows/pr_builds.yml; do
if [ -f "$f" ]; then
wc -l "$f"
else
echo "MISSING $f"
fi
done
echo
echo "== Relevant workflow excerpts around model download =="
python3 - <<'PY'
for path, ranges in [
(".github/workflows/ci.yml", [(430, 480), (400, 430)]),
(".github/workflows/pr_builds.yml", [(745, 805), (770, 810)]),
]:
print(f"--- {path} ---")
try:
lines = open(path, encoding="utf-8").readlines()
except FileNotFoundError:
print("MISSING")
continue
started = False
for i in range(max(1, min(r[0], len(lines))), min(r[1], len(lines)) + 1, 1):
# print line numbers in 1-based
print(f"{i:4d}\t{lines[i-1]}", end="")
print()
PY
echo
echo "== Search model fixture refs and pins/checksums =="
rg -n --hidden --glob '!*.{png,jpg,jpeg,gif}' --glob '!node_modules/**' \
'SKIPPY_CORRECTNESS|SKIPPY_CORRECTNESS_MODEL|skippy-correctness-model|Qwen3|Qwen3-0\.6B|Q4_K_M|hf download|huggingface-cli|revision|sha256|sha-256|checksum|checksums|digest' .github/workflows .agents/skills/manage-ci ci 2>/dev/null || true
echo
echo "== huggingface-cli download CLI availability/help =="
command -v huggingface-cli || true
huggingface-cli download --help 2>&1 | rg -n 'revision|local-dir|ignore|sha|checksum|verify|cache|allow|no-cache' || trueRepository: Mesh-LLM/mesh-llm
Length of output: 499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant workflow excerpts around model download =="
python3 - <<'PY'
for path, ranges in [
(".github/workflows/ci.yml", [(440, 475)]),
(".github/workflows/pr_builds.yml", [(760, 795)]),
]:
print(f"--- {path} ---")
try:
lines = open(path, encoding="utf-8").readlines()
except FileNotFoundError:
print("MISSING")
continue
for i, line in enumerate(lines, 1):
if 440 <= i <= 475 or 760 <= i <= 795:
print(f"{i:4d}\t{line}", end="")
PY
echo
echo "== Search model fixture refs and pins/checksums =="
for f in .github/workflows/ci.yml .github/workflows/pr_builds.yml .agents/skills/manage-ci ci/ci.md; do
[ -f "$f" ] && rg -n 'SKIPPY_CORRECTNESS|skippy-correctness-model|Qwen3|hf download|huggingface-cli|revision|sha256|sha-256|checksum|checksums|digest' "$f" || true
done
rg -n 'SKIPPY_CORRECTNESS|skippy-correctness-model|Qwen3|hf download|huggingface-cli|revision|sha256|sha-256|checksum|checksums|digest' ci/ .agents/skills/manage-ci || true
echo
echo "== huggingface-cli download CLI availability/help =="
command -v huggingface-cli || true
huggingface-cli download --help 2>&1 | rg -n 'revision|local-dir|ignore|sha|checksum|verify|cache|allow|no-cache' || trueRepository: Mesh-LLM/mesh-llm
Length of output: 28035
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect .agents/skills/manage-ci/SKILL.md inventory excerpt =="
sed -n '420,445p' .agents/skills/manage-ci/SKILL.md || true
echo
echo "== Inspect ci/ci.md correctness fixture contract excerpt =="
sed -n '260,275p' ci/ci.md || true
echo
echo "== Inspect huggingface_hub package metadata if preinstalled =="
python3 - <<'PY'
import importlib.util, sys, json, os, tempfile
if importlib.util.find_spec("huggingface_hub"):
mod = __import__("huggingface_hub")
pkg = os.path.dirname(mod.__file__)
try:
if os.path.exists(f"{pkg}/__pycache__/download.cpython-*.pyc"):
print("cached bytecode exists")
else:
print("no cached bytecode")
except Exception as e:
print("probe issue", repr(e))
else:
print("huggingface_hub not importable")
PYRepository: Mesh-LLM/mesh-llm
Length of output: 2345
🌐 Web query:
huggingface_hub hf download no revision defaults master latest documentation
💡 Result:
In the huggingface_hub library, the hf_hub_download and snapshot_download functions automatically default to the latest version available on the main branch of a repository [1][2]. You do not need to specify a revision if you want to download the latest files from the main branch [1]. If you need to download a specific version—such as from a different branch, a Git tag, or a specific commit hash—you must explicitly use the revision parameter [1][3]. Summary of behavior: - Default behavior: Automatically targets the latest revision of the main branch [1][4]. - Custom behavior: Use the revision parameter (e.g., revision="v1.0", revision="branch-name", or revision="commit-hash") to specify an alternative [1][2]. This applies to both hf_hub_download (for individual files) and snapshot_download (for entire repositories) [1][2].
Citations:
- 1: https://huggingface.co/docs/huggingface_hub/main/en/guides/download
- 2: https://huggingface.co/docs/huggingface_hub/en/guides/download
- 3: https://huggingface.co/docs/huggingface_hub/quick-start
- 4: https://huggingface.co/docs/huggingface_hub/guides/download
🌐 Web query:
huggingface_hub hf download local-dir default file name if already exists overwrite existing file behavior
💡 Result:
When using the local_dir parameter in the huggingface_hub library's hf_hub_download function, the library uses a metadata-driven approach rather than simply overwriting files based on their presence [1][2]. Key behaviors include: 1. Metadata Tracking: A.cache/huggingface/ folder is automatically created at the root of the specified local_dir [3][1]. This folder stores metadata about the downloaded files, which allows the library to track whether the local file is up-to-date with the remote version on the Hub [1][2]. 2. Preventing Redundant Downloads: If a file already exists in the local_dir, the library checks the stored metadata [1]. If the metadata indicates that the local file is current, the file is not re-downloaded [1][4]. 3. Updating Files: If the metadata reveals that the remote file has changed (i.e., it is not up-to-date), the library will download the new version of the file [1][2]. 4. Forcing Downloads: If you wish to force the library to overwrite an existing file regardless of its current state, you can set the force_download=True parameter in the hf_hub_download call [3][5]. In summary, the library is optimized to avoid unnecessary re-downloads by relying on internal metadata, ensuring that existing files are only overwritten when a newer version is detected or when a forced download is explicitly requested [1][4][2].
Citations:
- 1: https://huggingface.co/docs/huggingface_hub/main/en/guides/download
- 2: https://huggingface.co/docs/huggingface_hub/guides/download
- 3: https://huggingface.co/docs/huggingface_hub/main/package_reference/file_download
- 4: Revampt workflow when downloading to local dir huggingface/huggingface_hub#1738
- 5: https://huggingface.co/docs/huggingface_hub/en/package_reference/file_download
Pin and validate the Qwen3 correctness fixture in both workflows.
Both jobs download jc-builds/Qwen3-0.6B-Q4_K_M-GGUF with hf download and no --revision or checksum check. Set SKIPPY_CORRECTNESS_MODEL from that downloaded value, then lock it with a fixed --revision and verify the expected SHA-256 immediately before cargo test -p skippy-runtime uses it. Apply the same change to .github/workflows/pr_builds.yml#L777-L781 so model-backed CI does not depend on mutable upstream artifact bytes.
📍 Affects 2 files
.github/workflows/ci.yml#L461-L465(this comment).github/workflows/pr_builds.yml#L777-L781
🤖 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 @.github/workflows/ci.yml around lines 461 - 465, Pin and validate the Qwen3
correctness fixture in both workflows: in .github/workflows/ci.yml lines 461-465
and .github/workflows/pr_builds.yml lines 777-781, assign
SKIPPY_CORRECTNESS_MODEL to the downloaded model path, add the same fixed
--revision to hf download, and verify the expected SHA-256 immediately before
cargo test -p skippy-runtime uses it.
0255b7b to
112fa14
Compare
3 participants
Problem
A model could produce a valid native tool call, but parts of its meaning could be lost between chat rendering and verification:
For an OpenAI client using native tool calling, this could turn a valid tool call into malformed output, plain text, or a differently shaped message.
Fix
The change preserves content presence exactly, accepts tagged argument fields in either object order, and carries grammar state through sampled verification.
These fixes live at the native chat-parser and verification boundaries because those layers know the model's actual output grammar. Repairing the result later with text heuristics would lose that authority and introduce model-specific ambiguity.
Validation
OpenAI tests pass (154/154) and Skippy Server tests pass (353/353), including content-presence, argument-order, and grammar-state coverage.
Summary by CodeRabbit
null, and empty message content in OpenAI-compatible chat messages.