Split Skippy by functional boundary - #1194
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:
📝 WalkthroughWalkthroughThe PR reorganizes the Skippy llama.cpp patch queue, documents capability-owned native modules, adds modular ABI and model-package support, implements session state and staged execution, expands sampling and MTP support, and wires RPC, build, and GLM-DSA validation changes. ChangesSkippy runtime and patch queue
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SkippySession
participant SkippyExecution
participant LlamaContext
participant Sampler
Caller->>SkippySession: submit tokens or activation frames
SkippySession->>SkippyExecution: execute staged request
SkippyExecution->>LlamaContext: run filtered decode or verification
LlamaContext-->>SkippyExecution: return logits or activations
SkippyExecution->>Sampler: sample prediction or draft
Sampler-->>SkippyExecution: return token result
SkippyExecution-->>SkippySession: update position and history
SkippySession-->>Caller: return outputs and errors
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. |
![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 @.agents/skills/llama-stage-patch-changes/SKILL.md:
- Around line 69-76: Register an EXIT trap immediately after creating tmp_root
in the validation block to remove the temporary directory recursively. Keep the
existing prepare-llama.sh and build-llama.sh commands unchanged, ensuring
"$tmp_root" is cleaned up on both successful and failed runs.
- Around line 60-62: Update the patch-generation command to reuse the selected
llama.cpp checkout variable established by the alternate-checkout handling,
rather than hardcoding .deps/llama.cpp. Ensure git format-patch runs against
that variable while preserving the existing start-number, output-directory, and
HEAD arguments.
In `@docs/design/LLAMA_STAGE_INTEGRATION_PLAN.md`:
- Around line 13-18: Update the stale architecture section and its references to
ExternalLlamaBackend, llama-server, and rpc-server so they match the shipped
design, or clearly label them as historical. Remove guidance that presents the
external runtime lane as current, ensuring maintainers are directed only to the
current architecture.
🪄 Autofix
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: e15e02a4-d7e1-4a37-977a-76fe72a280cb
📒 Files selected for processing (7)
.agents/skills/llama-stage-patch-changes/SKILL.md.agents/skills/skippy-correctness/SKILL.md.agents/skills/skippy-model-package/SKILL.mdAGENTS.mddocs/SKIPPY.mddocs/design/LLAMA_STAGE_INTEGRATION_PLAN.mdthird_party/llama.cpp/patches/0066-Split-Skippy-by-functional-boundary.patch
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (10)
third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch (5)
376-376: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the misleading
(void) request_logits;.
request_logitsis used at Line 398 and Line 426. The cast states the opposite and is residue from the translation-unit split.♻️ Proposed change
- (void) request_logits; -🤖 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/0009-Add-Skippy-sampling-and-speculative-decoding.patch` at line 376, Remove the unnecessary `(void) request_logits;` statement from the affected function, leaving the existing uses of request_logits unchanged.
892-896: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe candidate array is reallocated on every sampled token.
Lines 892-896 build a fresh
std::vector<llama_token_data>ofn_vocabentries for each call.skippy_chat_sample_tokenruns once per generated token. For a 150k vocabulary that is roughly 1.8 MB allocated, filled, and freed per token on the decode path.Hold the buffer on
skippy_sessionand refill it in place. The allocation then happens once per session.♻️ Sketch of the change
- std::vector<llama_token_data> candidates; - candidates.reserve(static_cast<size_t>(n_vocab)); - for (llama_token token = 0; token < n_vocab; ++token) { - candidates.push_back({ token, logits[token], 0.0f }); - } + // `sampling_candidates` is a per-session scratch buffer added to skippy_session. + std::vector<llama_token_data> & candidates = session->sampling_candidates; + candidates.resize(static_cast<size_t>(n_vocab)); + for (llama_token token = 0; token < n_vocab; ++token) { + candidates[static_cast<size_t>(token)] = { token, logits[token], 0.0f }; + }🤖 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/0009-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 892 - 896, Update skippy_chat_sample_token to reuse a candidate buffer stored on skippy_session instead of constructing a local std::vector<llama_token_data> for each sampled token. Allocate or reserve the buffer once per session, then clear and refill it in place from logits while preserving the existing candidate contents and sampling behavior.
542-575: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
skippy_compute_token_signal_contextmakes three full vocabulary passes.The function scans
n_vocabat Lines 542-556 for the top two logits, again at Lines 564-566 forsum_exp, and again at Lines 571-575 for entropy.skippy_record_signalruns on every decoded token, andn_vocabreaches six figures for current models. This is a per-token decode cost.Fold the entropy accumulation into the
sum_exppass. Entropy equalslog_sum_exp - (sum of p_i * logit_i), and both sums can be accumulated together over unnormalised exponentials.♻️ Proposed change
double sum_exp = 0.0; - double entropy = 0.0; + double sum_exp_logit = 0.0; for (int32_t token = 0; token < n_vocab; ++token) { - sum_exp += std::exp(static_cast<double>(logits[token] - max_logit)); + const double e = std::exp(static_cast<double>(logits[token] - max_logit)); + sum_exp += e; + sum_exp_logit += e*static_cast<double>(logits[token]); } if (sum_exp <= 0.0 || !std::isfinite(sum_exp)) { return false; } const double log_sum_exp = static_cast<double>(max_logit) + std::log(sum_exp); - for (int32_t token = 0; token < n_vocab; ++token) { - const double logprob = static_cast<double>(logits[token]) - log_sum_exp; - const double prob = std::exp(logprob); - entropy -= prob * logprob; - } + const double entropy = log_sum_exp - sum_exp_logit/sum_exp;🤖 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/0009-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 542 - 575, Optimize skippy_compute_token_signal_context by combining the sum_exp and entropy-related calculations into a single vocabulary pass after the top-logit scan. Accumulate unnormalized exponentials and their logit-weighted sum, then compute entropy as log_sum_exp minus the normalized weighted-logit sum, preserving the existing validation and signal results.
151-167: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
skippy_ngram_cache_draftcopies the whole history on every call.Line 151 copies
cache->historyintoinput. Drafting runs once per generated token, so the cost is O(history) per token and O(history²) per request. For long contexts this is a measurable decode-path cost.The copy exists only so the continuation prefix can be appended. Append to
cache->historydirectly, draft, then truncate back to the committed size.♻️ Proposed change
- std::vector<llama_token> input = cache->history; std::vector<llama_token> draft; + const size_t committed_size = cache->history.size(); if (continuation_prefix_count == 0) { - draft.push_back(input.back()); + draft.push_back(cache->history.back()); } else { - input.insert(input.end(), continuation_prefix, continuation_prefix + continuation_prefix_count); + cache->history.insert( + cache->history.end(), continuation_prefix, continuation_prefix + continuation_prefix_count); draft.push_back(continuation_prefix[continuation_prefix_count - 1]); } common_ngram_cache_draft( - input, + cache->history, draft, max_draft_tokens, cache->ngram_min, cache->ngram_max, cache->context, cache->dynamic, cache->static_cache); + cache->history.resize(committed_size);Confirm that
common_ngram_cache_draftdoes not mutate itsinpargument before applying this change.🤖 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/0009-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 151 - 167, Update skippy_ngram_cache_draft to avoid copying cache->history on every call: record its committed size, append the continuation prefix directly to cache->history when present, invoke common_ngram_cache_draft with the history, then restore the original size before returning. Preserve the existing draft-token selection for both prefix and no-prefix cases, and confirm common_ngram_cache_draft does not mutate its input before relying on this approach.
1266-1274: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftWrap the manual
batch.tokenlifetime in an RAII guard.The code allocates
batch.tokenwithstd::mallocover the null pointer thatllama_batch_initleaves whenn_embd > 0. Every exit path must then runstd::free(batch.token), setbatch.token = nullptr, and only then callllama_batch_free(batch). That sequence is repeated at Lines 1294-1297, 1356-1360, 1500-1502, and 1517-1519.The correctness of each path depends on an undocumented
llama_batch_initinvariant and on remembering the exact order. A new early return that omits one step causes a leak or a double free.Introduce a small scope guard that owns both the batch and the token allocation, in the same style as
skippy_mtp_depth_scopeandskippy_mtp_embeddings_scopein this file.🤖 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/0009-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 1266 - 1274, Introduce an RAII scope guard near the batch setup that owns the manually allocated batch.token and the llama_batch from llama_batch_init, following the style of skippy_mtp_depth_scope and skippy_mtp_embeddings_scope. Ensure its destructor frees batch.token, sets it to nullptr, and then calls llama_batch_free(batch); replace the repeated cleanup at the identified return paths so all exits use this guard without manual frees.third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch (2)
1602-1618: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the positional
llama_kv_cacheconstructor arguments.The constructor takes
true, false, true, 1, 1, 1, 0with no indication of meaning. The same literal block is repeated three times in this test at Lines 1602, 1684, and 1734. If the constructor parameter order changes, the test still compiles and silently tests a different configuration.Use the
/*name =*/comment convention that this patch set already uses forllama_batchinitialisers.🤖 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/0006-Add-Skippy-session-and-state-management.patch` around lines 1602 - 1618, Annotate the positional llama_kv_cache constructor arguments with /*name =*/ comments, using the constructor’s parameter names and matching the existing llama_batch initializer convention. Apply the annotations consistently to all three repeated llama_kv_cache constructions in this test, without changing their values or order.
515-521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe catch blocks report success after a grammar setup failure.
Both handlers clear chat sampling and return
skippy_success(out_error). The caller cannot distinguish "no grammar was requested" from "grammar configuration failed". Generation then continues without the requested grammar constraint, and the output can violate the caller's schema.Log the exception, or return a distinct status so the caller can decide. Note that
eis unused in the first handler, which suggests the message was intended to be reported.♻️ Suggested change to surface the failure
} catch (const std::exception & e) { + fprintf(stderr, "skippy: chat sampling configuration failed: %s\n", e.what()); skippy_clear_chat_sampling(session); return skippy_success(out_error); } catch (...) { + fprintf(stderr, "skippy: chat sampling configuration failed: unknown native exception\n"); skippy_clear_chat_sampling(session); return skippy_success(out_error); }🤖 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/0006-Add-Skippy-session-and-state-management.patch` around lines 515 - 521, Update the grammar-setup exception handlers in the session/state management flow to surface failure instead of returning skippy_success. Use the named std::exception variable e to log the error, handle unknown exceptions consistently, and return the established distinct failure status so callers do not continue generation without the requested grammar constraint.third_party/llama.cpp/patches/0011-Wire-staged-runtime-builds-and-tests.patch (3)
220-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the copy and move operations of
scoped_test_env.The destructor restores the environment variable. A copy would restore it twice and could restore a stale value after the second object is destroyed. The class holds a raw
namepointer, so a copy also aliases that pointer.♻️ Proposed change
class scoped_test_env { public: scoped_test_env(const char * name, const char * value) : name(name) {+ scoped_test_env(const scoped_test_env &) = delete; + scoped_test_env & operator=(const scoped_test_env &) = delete; + ~scoped_test_env() {🤖 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/0011-Wire-staged-runtime-builds-and-tests.patch` around lines 220 - 243, Delete the copy and move constructors and assignment operators from scoped_test_env, making instances non-copyable and non-movable. Preserve the existing constructor and destructor behavior for scoped environment restoration.
95-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
llama_build_and_testfor the two new flash-attn tests.Every other test in
tests/CMakeLists.txtuses thellama_build/llama_build_and_testhelpers. These two targets calladd_executableandadd_testdirectly. The helpers apply the project-wide compile options, labels, and working directory. Direct targets miss them and drift over time.If the tests must link only
ggmland notllama, state that in a comment next to the targets.🤖 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/0011-Wire-staged-runtime-builds-and-tests.patch` around lines 95 - 101, The new test targets test-flash-attn-bias and test-flash-attn-generic-hash should use the project’s llama_build_and_test helper instead of direct add_executable, target_link_libraries, and add_test calls, so they inherit standard compile options, labels, and working-directory configuration. Preserve their ggml-only linkage; if the helper requires clarification, add a nearby comment stating that they must not link llama.
527-539: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
n_tokensassignment.
common_batch_addincrementsbatch.n_tokensfor each token. Line 533 then sets the same value. If the loop and the assignment ever disagree, the assignment silently wins.♻️ Proposed change
- batch.n_tokens = tokens.size(); if (llama_decode(lctx, batch)) {🤖 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/0011-Wire-staged-runtime-builds-and-tests.patch` around lines 527 - 539, Remove the redundant batch.n_tokens assignment from decode_tokens after the common_batch_add loop; rely on common_batch_add to maintain the token count before calling llama_decode.
🤖 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 @.agents/skills/llama-patch-changes/SKILL.md:
- Around line 42-52: Update the deliberate rewrite instructions in
.agents/skills/llama-patch-changes/SKILL.md, lines 42-52, to generate patches
from the selected authoritative checkout by invoking git format-patch with git
-C "$llama_checkout" and using that checkout’s upstream.txt and patches
directory. Update the ordinary workflow reference in
.agents/skills/llama-stage-patch-changes/SKILL.md, lines 54-55, so its hardcoded
.deps/llama.cpp path can be overridden by the selected llama checkout; preserve
the existing patch-generation behavior otherwise.
In @.agents/skills/llama-stage-patch-changes/SKILL.md:
- Around line 21-25: Update the patch series so patch 0005 contains only the
Skippy ABI capability, moving model lifecycle and loading changes into a
separate contiguous capability-owned patch. Adjust subsequent patch numbers and
references consistently, or document the combined ABI/lifecycle patch as an
explicit exception in the patching guidance.
In
`@third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch`:
- Around line 350-361: Synchronize skippy_model::lane_in_use ownership with a
mutex: guard the claim loop in skippy_session_create
(third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch
lines 350-361), guard the identical claim loop in
skippy_session_create_from_resident_prefix (lines 1450-1461) while retaining the
lock through rollback writes at lines 1469, 1478, 1492, and 1501, and guard lane
release in skippy_session_free (lines 592-597) with the same mutex.
Alternatively, document a single-threaded FFI lifecycle contract on skippy_model
and remove the concern.
- Around line 1174-1180: Update skippy_retire_verify_checkpoint to validate
session->ctx is non-null immediately after the existing session check and return
SKIPPY_STATUS_INVALID_ARGUMENT via skippy_set_error when it is missing, before
calling get_memory().
- Around line 812-815: Validate imported_cells against the context size before
using it to update session state, rejecting or ignoring values greater than the
configured context limit so session->n_past cannot become arbitrary. Add a
comment documenting that the bytes at this offset represent the wire-format
imported cell count, and apply the same validation consistently in
skippy_import_state and skippy_import_full_state.
In `@third_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patch`:
- Around line 234-283: Trim the duplicated include block in each translation
unit defining skippy_decode_step_frame_batch_sampled to only the headers
required by that function and its direct capability dependencies. Remove unused
JSON, regex, file-stream, set, unordered_set, thread, model-loader, gguf, and
sysctl includes, along with the using json alias. Apply the same dependency
cleanup in execution-single.cpp and verification.cpp while preserving headers
actually referenced by each TU.
- Around line 1439-1453: Update the stop_after_first_mismatch condition in the
token-processing loop to apply whenever sampler state exists, not only when
session->grammar_sampler is non-null. Preserve the existing mismatch detection,
out_token_count update, and early break so rejected suffixes are excluded from
skippy_record_tokens and downstream sampling-history synchronization.
In
`@third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch`:
- Around line 1071-1077: Update the translation unit’s include block to
explicitly include the standard headers declaring std::memcpy and
std::malloc/std::free: add <cstring> and <cstdlib> alongside the existing
standard-library includes.
- Around line 949-991: Replace the duplicated sampler-chain construction in the
fallback path with a call to the existing skippy_build_sampling_chain helper.
Preserve the existing nullptr fallback and token-history acceptance behavior,
using the helper’s returned sampler so chain configuration remains centralized.
- Around line 923-947: Update skippy_sample_token_ith to apply one session null
check at the function entry, then remove the redundant session != nullptr checks
from the fast-path and sampling-chain conditions. Ensure all subsequent calls,
including skippy_greedy_sample_ith and llama_synchronize(session->ctx), execute
only with a valid session.
- Around line 408-437: Reorder post-decode handling in the multi-token path, the
single-token path, and skippy_verify_token_batch so skippy_record_tokens runs
immediately after a successful skippy_decode_batch, before
skippy_mtp_sync_target_inputs. Preserve and return the MTP sync error afterward,
ensuring committed target tokens remain recorded even when synchronization
fails.
In
`@third_party/llama.cpp/patches/0011-Wire-staged-runtime-builds-and-tests.patch`:
- Around line 372-377: Update glm_dsa_graph_op_counter_callback so counts->total
is incremented only after the ask == true guard, matching the other counters and
counting each accepted node once; preserve the existing true return for non-ask
callbacks.
- Around line 245-269: Update
test_glm_dsa_rejects_conflicting_indexshare_metadata and
test_glm_dsa_rejects_shared_first_indexshare_metadata so a nullptr from
llama_model_init_from_user is treated as failure rather than success. Only
return true when an exception message contains the expected rejection error;
preserve false for all non-throwing or unrelated failures.
- Around line 1141-1167: Update the GLM_DSA contract test chain in test_backends
to evaluate every check independently and accumulate failures instead of using
short-circuit &&. Avoid calling glm_dsa_cpu_devs() as an unguarded argument:
reuse the protected CPU-device handling from
test_glm_dsa_cpu_compact_decode_graph or otherwise catch/report the
runtime_error before invoking CPU-dependent tests. Remove the duplicate
test_glm_dsa_compact_decode_graph(seed, glm_dsa_cpu_devs()) invocation while
preserving the existing all_ok failure reporting.
---
Nitpick comments:
In
`@third_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patch`:
- Around line 1602-1618: Annotate the positional llama_kv_cache constructor
arguments with /*name =*/ comments, using the constructor’s parameter names and
matching the existing llama_batch initializer convention. Apply the annotations
consistently to all three repeated llama_kv_cache constructions in this test,
without changing their values or order.
- Around line 515-521: Update the grammar-setup exception handlers in the
session/state management flow to surface failure instead of returning
skippy_success. Use the named std::exception variable e to log the error, handle
unknown exceptions consistently, and return the established distinct failure
status so callers do not continue generation without the requested grammar
constraint.
In
`@third_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patch`:
- Line 376: Remove the unnecessary `(void) request_logits;` statement from the
affected function, leaving the existing uses of request_logits unchanged.
- Around line 892-896: Update skippy_chat_sample_token to reuse a candidate
buffer stored on skippy_session instead of constructing a local
std::vector<llama_token_data> for each sampled token. Allocate or reserve the
buffer once per session, then clear and refill it in place from logits while
preserving the existing candidate contents and sampling behavior.
- Around line 542-575: Optimize skippy_compute_token_signal_context by combining
the sum_exp and entropy-related calculations into a single vocabulary pass after
the top-logit scan. Accumulate unnormalized exponentials and their
logit-weighted sum, then compute entropy as log_sum_exp minus the normalized
weighted-logit sum, preserving the existing validation and signal results.
- Around line 151-167: Update skippy_ngram_cache_draft to avoid copying
cache->history on every call: record its committed size, append the continuation
prefix directly to cache->history when present, invoke common_ngram_cache_draft
with the history, then restore the original size before returning. Preserve the
existing draft-token selection for both prefix and no-prefix cases, and confirm
common_ngram_cache_draft does not mutate its input before relying on this
approach.
- Around line 1266-1274: Introduce an RAII scope guard near the batch setup that
owns the manually allocated batch.token and the llama_batch from
llama_batch_init, following the style of skippy_mtp_depth_scope and
skippy_mtp_embeddings_scope. Ensure its destructor frees batch.token, sets it to
nullptr, and then calls llama_batch_free(batch); replace the repeated cleanup at
the identified return paths so all exits use this guard without manual frees.
In
`@third_party/llama.cpp/patches/0011-Wire-staged-runtime-builds-and-tests.patch`:
- Around line 220-243: Delete the copy and move constructors and assignment
operators from scoped_test_env, making instances non-copyable and non-movable.
Preserve the existing constructor and destructor behavior for scoped environment
restoration.
- Around line 95-101: The new test targets test-flash-attn-bias and
test-flash-attn-generic-hash should use the project’s llama_build_and_test
helper instead of direct add_executable, target_link_libraries, and add_test
calls, so they inherit standard compile options, labels, and working-directory
configuration. Preserve their ggml-only linkage; if the helper requires
clarification, add a nearby comment stating that they must not link llama.
- Around line 527-539: Remove the redundant batch.n_tokens assignment from
decode_tokens after the common_batch_add loop; rely on common_batch_add to
maintain the token count before calling llama_decode.
🪄 Autofix
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: 3baa2e27-7450-4b44-89b0-26b878fd2d23
📒 Files selected for processing (83)
.agents/skills/llama-patch-changes/SKILL.md.agents/skills/llama-stage-patch-changes/SKILL.mdAGENTS.mddocs/design/LLAMA_STAGE_INTEGRATION_PLAN.mddocs/skippy/family/qwen-results.mdthird_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patchthird_party/llama.cpp/patches/0001-Add-Skippy-ABI-and-package-writer-foundation.patchthird_party/llama.cpp/patches/0002-Add-early-staged-model-family-and-chat-support.patchthird_party/llama.cpp/patches/0002-Add-staged-model-graph-and-family-support.patchthird_party/llama.cpp/patches/0003-Add-GLM-DSA-backend-execution-support.patchthird_party/llama.cpp/patches/0003-Add-staged-sampling-checkpoints-and-part-loading.patchthird_party/llama.cpp/patches/0004-Add-Inkling-model-and-multimodal-support.patchthird_party/llama.cpp/patches/0004-Add-lanes-external-media-and-chat-grammar-support.patchthird_party/llama.cpp/patches/0005-Add-Skippy-ABI-and-model-lifecycle.patchthird_party/llama.cpp/patches/0005-Add-resident-prefix-cache-and-session-refinements.patchthird_party/llama.cpp/patches/0006-Add-Skippy-session-and-state-management.patchthird_party/llama.cpp/patches/0006-Expand-staged-execution-across-dense-and-recurrent-f.patchthird_party/llama.cpp/patches/0007-Add-Skippy-activation-frame-handling.patchthird_party/llama.cpp/patches/0007-Expand-staged-execution-across-VL-and-broad-model-fa.patchthird_party/llama.cpp/patches/0008-Add-Skippy-staged-execution-paths.patchthird_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patchthird_party/llama.cpp/patches/0009-Add-Skippy-sampling-and-speculative-decoding.patchthird_party/llama.cpp/patches/0009-Add-chat-grammar-device-enumeration-and-runtime-even.patchthird_party/llama.cpp/patches/0010-Add-MTP-execution-support-and-sampling-cleanup.patchthird_party/llama.cpp/patches/0010-Add-Skippy-tokenization-and-stage-chat.patchthird_party/llama.cpp/patches/0011-Pass-reasoning-format-through-stage-chat-templates.patchthird_party/llama.cpp/patches/0011-Wire-staged-runtime-builds-and-tests.patchthird_party/llama.cpp/patches/0012-Wire-mmap-and-mlock-runtime-load-options.patchthird_party/llama.cpp/patches/0013-Add-external-MTP-draft-sidecar-attachment.patchthird_party/llama.cpp/patches/0014-Add-non-frame-native-MTP-decode-ABI.patchthird_party/llama.cpp/patches/0015-Fix-stage-activation-graph-input-allocation.patchthird_party/llama.cpp/patches/0016-Recognize-thinking-field-in-chat-auto-parser.patchthird_party/llama.cpp/patches/0017-Expose-stateful-N-gram-cache-ABI.patchthird_party/llama.cpp/patches/0018-Remove-legacy-session-checkpoint-ABI.patchthird_party/llama.cpp/patches/0019-Re-prime-native-MTP-after-state-restoration.patchthird_party/llama.cpp/patches/0020-Fix-N-gram-confidence-threshold-indexing.patchthird_party/llama.cpp/patches/0023-tests-cover-native-GLM-DSA-execution-paths.patchthird_party/llama.cpp/patches/0024-Support-GLM-DSA-fused-KV_B-tensors.patchthird_party/llama.cpp/patches/0025-Run-GLM-DSA-through-IndexShare-graph.patchthird_party/llama.cpp/patches/0026-Bump-Skippy-ABI-for-GLM-DSA-runtime-config.patchthird_party/llama.cpp/patches/0027-Fix-GLM-DSA-Metal-get_rows-placement.patchthird_party/llama.cpp/patches/0028-ggml-default-GLM-MoE-two-phase-Metal-path.patchthird_party/llama.cpp/patches/0029-ggml-add-GLM-MoE-Metal-selector-diagnostics.patchthird_party/llama.cpp/patches/0030-ggml-skip-zero-weight-GLM-MoE-gate-up-slots.patchthird_party/llama.cpp/patches/0031-tests-cover-full-GLM-MoE-selected-chain.patchthird_party/llama.cpp/patches/0032-tests-add-GLM-Q2Q3-selected-weight-roofline.patchthird_party/llama.cpp/patches/0033-ggml-add-active-count-Q3-GLM-MoE-down-kernels.patchthird_party/llama.cpp/patches/0034-ggml-use-GLM-max-active-policy-for-Q3-down-kernels.patchthird_party/llama.cpp/patches/0035-ggml-honor-explicit-Q2-gate-up-Metal-variant-flags.patchthird_party/llama.cpp/patches/0036-ggml-make-GLM-MoE-roofline-honor-active-experts.patchthird_party/llama.cpp/patches/0037-ggml-avoid-GLM-Q3-fused-tail-under-active-policy.patchthird_party/llama.cpp/patches/0038-ggml-shrink-GLM-Q2-gate-up-active-dispatch.patchthird_party/llama.cpp/patches/0039-ggml-skip-inactive-GLM-Q3-down-slots.patchthird_party/llama.cpp/patches/0040-Fix-GLM-DSA-native-MTP-execution.patchthird_party/llama.cpp/patches/0041-Adapt-Skippy-model-loading-to-load-modes.patchthird_party/llama.cpp/patches/0042-Harden-staged-session-and-sideband-bookkeeping.patchthird_party/llama.cpp/patches/0043-Correct-GLM-DSA-staged-graph-contracts.patchthird_party/llama.cpp/patches/0044-ggml-metal-make-small-batch-matmul-batch-invariant.patchthird_party/llama.cpp/patches/0045-Fix-GLM-DSA-Metal-dispatch-safety.patchthird_party/llama.cpp/patches/0046-Fix-empty-transposed-KV-page-import-and-export.patchthird_party/llama.cpp/patches/0047-Scope-GLM-DSA-backend-test-environment.patchthird_party/llama.cpp/patches/0048-Fix-sampled-verification-token-history.patchthird_party/llama.cpp/patches/0048-Support-Laguna-staged-execution.patchthird_party/llama.cpp/patches/0049-Accept-tagged-tool-arguments-in-object-order.patchthird_party/llama.cpp/patches/0049-Filter-staged-runtime-memory-to-layer-range.patchthird_party/llama.cpp/patches/0050-Add-TML-Inkling-architecture.patchthird_party/llama.cpp/patches/0050-Preserve-OpenAI-message-content-presence.patchthird_party/llama.cpp/patches/0051-Add-staged-execution-support-for-Inkling.patchthird_party/llama.cpp/patches/0052-Advance-native-MTP-depth-per-draft-step.patchthird_party/llama.cpp/patches/0053-Replay-multi-depth-MTP-caches-correctly.patchthird_party/llama.cpp/patches/0054-Add-Inkling-multi-depth-MTP-sidecars.patchthird_party/llama.cpp/patches/0055-Carry-multimodal-Inkling-embeddings-into-MTP.patchthird_party/llama.cpp/patches/0056-Size-recurrent-memory-for-appended-MTP-layers.patchthird_party/llama.cpp/patches/0057-Complete-Inkling-and-GLM-metadata-integration.patchthird_party/llama.cpp/patches/0058-skippy-balance-filtered-stages-across-devices.patchthird_party/llama.cpp/patches/0059-Preserve-hybrid-verify-window-state-across-trim.patchthird_party/llama.cpp/patches/0060-skippy-retire-accepted-verify-checkpoints-exactly.patchthird_party/llama.cpp/patches/0061-Reject-trims-after-failed-verify-recovery.patchthird_party/llama.cpp/patches/0062-Harden-Inkling-MTP-and-KV-contiguity-state.patchthird_party/llama.cpp/patches/0063-Harden-Inkling-MTP-sidecar-and-verify-cleanup.patchthird_party/llama.cpp/patches/0064-Accept-Inkling-arguments-tool-call-key.patchthird_party/llama.cpp/patches/0065-Fix-role-only-OpenAI-chat-test-expectation.patchthird_party/llama.cpp/patches/0066-Stop-chat-grammar-verification-at-the-first-mismatch.patch
💤 Files with no reviewable changes (8)
- third_party/llama.cpp/patches/0006-Expand-staged-execution-across-dense-and-recurrent-f.patch
- third_party/llama.cpp/patches/0004-Add-lanes-external-media-and-chat-grammar-support.patch
- third_party/llama.cpp/patches/0005-Add-resident-prefix-cache-and-session-refinements.patch
- third_party/llama.cpp/patches/0008-Add-external-decode-media-prefill-and-newer-family-s.patch
- third_party/llama.cpp/patches/0009-Add-chat-grammar-device-enumeration-and-runtime-even.patch
- third_party/llama.cpp/patches/0002-Add-early-staged-model-family-and-chat-support.patch
- third_party/llama.cpp/patches/0003-Add-staged-sampling-checkpoints-and-part-loading.patch
- third_party/llama.cpp/patches/0001-Add-Skippy-ABI-and-package-writer-foundation.patch
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/design/LLAMA_STAGE_INTEGRATION_PLAN.md
- AGENTS.md
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
third_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch (1)
581-585: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not select ACCEL devices for GLM-DSA sparse tests.
Line 585 selects
GGML_BACKEND_DEVICE_TYPE_ACCEL.glm_dsa_backend_config_supportedat Lines 1122-1134 rejects the same device type for GLM-DSA. A host with an ACCEL device can therefore run tests on a configuration that the backend matrix marks unsupported.Restrict this helper to GPU and integrated GPU devices, or add ACCEL support to the GLM-DSA backend contract consistently.
Proposed fix
case GGML_BACKEND_DEVICE_TYPE_GPU: case GGML_BACKEND_DEVICE_TYPE_IGPU: - case GGML_BACKEND_DEVICE_TYPE_ACCEL: return {dev};🤖 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/0012-Wire-staged-runtime-builds-and-tests.patch` around lines 581 - 585, Update the device-type switch in the GLM-DSA sparse-test helper to select only GPU and IGPU devices; remove GGML_BACKEND_DEVICE_TYPE_ACCEL so test selection matches glm_dsa_backend_config_supported.third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch (3)
630-665: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
skippy_session_freeleaks the samplers on the preserve-prefix path.
skippy_sessionowns two raw pointers,sampling_chainandgrammar_sampler.skippy_clear_chat_samplingis the only function that frees them, andskippy_session_freereaches it only throughskippy_session_resetat Line 649.Two paths skip that call and then run
delete session:
session->preserve_prefix_on_freeis true,borrowed_sequenceis false, andstage_modelis set.skippy_session_save_prefixsetspreserve_prefix_on_freeto true at Line 1448 ofsrc/skippy/state.cpp.session->ctxis null.A server that saves a resident prefix and also calls
skippy_session_configure_chat_samplingtherefore leaks one sampler chain and one grammar sampler per session. Free the samplers unconditionally before the delete.🛡️ Proposed fix
if (!session->borrowed_sequence) { skippy_release_execution_lane(session->stage_model, session->seq_id); } + skippy_clear_chat_sampling(session); skippy_mtp_clear_session_state(session); delete session;
skippy_clear_chat_samplingmust be idempotent, becauseskippy_session_resetmay already have called it on the other path.🤖 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/0007-Add-Skippy-session-and-state-management.patch` around lines 630 - 665, Update skippy_session_free to call skippy_clear_chat_sampling unconditionally before delete session, including preserve-prefix and null-context paths. Ensure skippy_clear_chat_sampling is idempotent so this remains safe when skippy_session_reset has already cleared the samplers.
581-588: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe exception handlers report success after they discard the chat sampling configuration.
Both
catchblocks callskippy_clear_chat_sampling(session)and thenreturn skippy_success(out_error).skippy_successsets*out_errortonullptrand returnsSKIPPY_STATUS_OK.A malformed
metadata_json, a JSON parse failure, or an exception fromskippy_build_sampling_chaintherefore looks identical to a successful configuration. The caller believes the grammar and sampling config are active. The session then samples without them. The Rust FFI has no way to detect this.The non-exception failure path at Lines 577-580 returns a real status, so the two paths disagree. Return an error from the handlers, and include the exception text.
🐛 Proposed fix
} catch (const std::exception & e) { skippy_clear_chat_sampling(session); - return skippy_success(out_error); + skippy_disable_chat_grammar_after_exception(session, "chat sampling configuration", e.what()); + skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, e.what()); + return SKIPPY_STATUS_INVALID_ARGUMENT; } catch (...) { skippy_clear_chat_sampling(session); - return skippy_success(out_error); + skippy_set_error(out_error, SKIPPY_STATUS_RUNTIME_ERROR, "unknown exception while configuring chat sampling"); + return SKIPPY_STATUS_RUNTIME_ERROR; }If a lenient fallback is intentional, state that in a comment and confirm the Rust caller expects it.
🤖 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/0007-Add-Skippy-session-and-state-management.patch` around lines 581 - 588, Update the exception handlers in the session configuration flow to return a failure status instead of skippy_success after skippy_clear_chat_sampling(session). For the std::exception handler, include e.what() in the error reported through out_error; ensure the catch-all handler also reports a real failure consistently with the existing non-exception failure path.
723-741: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
repetition_countand the entropy metrics can cover different token spans.The entropy and margin statistics use
count, derived fromsignal_history. The repetition scan usestoken_count, derived fromtoken_history. These two vectors can hold different numbers of entries.
skippy_session_restore_prefixinsrc/skippy/state.cppassignstoken_historyfrom the prefix at Line 1500 and clearssignal_historyat Line 1502. After later decoding,token_historystays longer thansignal_historyby the prefix length. A caller that requests a 32-token window then receives entropy over the last 32 generated tokens and repetitions over the last 32 tokens of a longer history.out_window->token_countreports only the signal span, so the caller cannot detect the mismatch.Scan the same span for both metrics.
🐛 Proposed fix
uint32_t repetition_count = 0; if (!session->token_history.empty()) { - const size_t token_count = std::min<size_t>(window_tokens, session->token_history.size()); + const size_t token_count = std::min<size_t>(count, session->token_history.size()); const size_t token_start = session->token_history.size() - token_count;🤖 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/0007-Add-Skippy-session-and-state-management.patch` around lines 723 - 741, Align repetition_count with the signal-history window used for the entropy and margin metrics. In the window-statistics code, derive the repetition scan’s start and length from count and the corresponding signal span rather than token_history.size(), while preserving safe bounds handling; ensure all out_window fields describe the same trailing window.third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch (6)
1809-1880: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove the output file when the tensor copy fails.
skippy_copy_source_tensorswrites the GGUF metadata tooutput_pathat Line 1814, then appends tensor data. If any read, write, orfclosefails, the function returnsSKIPPY_STATUS_IO_ERRORand leaves a truncated GGUF atoutput_path.Both callers,
skippy_write_slice_ggufandskippy_write_gguf_from_parts, propagate the status without cleanup. A later tool that reads the path sees a file with valid metadata and missing or partial tensor data. Delete the partial file before returning the error.🛡️ Proposed fix
if (!ok) { + std::remove(output_path); skippy_set_error(out_error, SKIPPY_STATUS_IO_ERROR, "failed to copy selected GGUF tensor data"); return SKIPPY_STATUS_IO_ERROR; }Apply the same cleanup to the metadata-write failure at Line 1814.
🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 1809 - 1880, Update skippy_copy_source_tensors so every failure path after creating output_path removes the partial output file before returning SKIPPY_STATUS_IO_ERROR, including the initial gguf_write_to_file failure and the later read, write, seek, or fclose failures. Reuse the existing output_path value and preserve the current error reporting and success behavior.
180-194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
skippy_env_u32truncates before it clamps.
std::strtoulreturnsunsigned long, which is 64-bit on Linux and macOS. The code casts touint32_tfirst and clamps afterwards. An input of4294967297truncates to1and then clamps tomin_value. An input of-1wraps toULONG_MAX, truncates toUINT32_MAX, and clamps tomax_valueinstead of returningfallback. Clamp on the wide type, and reject values thatstrtoulreports as negative.🐛 Proposed fix
char * end = nullptr; errno = 0; + while (*value == ' ' || *value == '\t') { + ++value; + } + if (*value == '-') { + return fallback; + } const unsigned long parsed = std::strtoul(value, &end, 10); if (errno != 0 || end == value || *end != '\0') { return fallback; } - return std::min<uint32_t>(max_value, std::max<uint32_t>(min_value, static_cast<uint32_t>(parsed))); + const unsigned long clamped = std::min<unsigned long>( + static_cast<unsigned long>(max_value), + std::max<unsigned long>(static_cast<unsigned long>(min_value), parsed)); + return static_cast<uint32_t>(clamped);🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 180 - 194, Update skippy_env_u32 to validate the parsed unsigned-long value before converting it to uint32_t: reject inputs where strtoul reports a negative value, clamp the wide parsed value to the uint32_t bounds and configured min_value/max_value, then cast only after clamping; retain fallback handling for parse and range errors.
1194-1198: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not expose a heap address as
model_id.
reinterpret_cast<uint64_t>(stage_model)publishes the raw heap address of theskippy_modelallocation in every runtime event. The event travels across the FFI boundary to the Rust consumer, which may log it or forward it. That discloses the process memory layout and weakens ASLR. The value is also unstable across reallocation, so it is a poor identifier.Use a monotonic counter instead.
🔒️ Proposed fix
+static uint64_t skippy_next_model_id() { + static std::atomic<uint64_t> counter{0}; + return counter.fetch_add(1, std::memory_order_relaxed) + 1; +}if (event_scope != nullptr) { - event_scope->set_model_id(reinterpret_cast<uint64_t>(stage_model)); + event_scope->set_model_id(skippy_next_model_id()); skippy_emit_observable_backend_device(event_scope, config, model); event_scope->emit_finished(); }This needs
#include <atomic>.🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 1194 - 1198, Replace the raw pointer value assigned by the event_scope model-id path with a process-safe monotonic counter, and add the required atomic support. Ensure each model receives a stable unique numeric identifier without exposing stage_model’s heap address, while preserving the existing event emission flow in the surrounding lifecycle code.
1576-1592: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe manual GGUF parser trusts untrusted header fields.
skippy_parse_tensor_metadatareads a model file that the process does not control. Three values are used without validation:
- Line 1590:
out.resize(static_cast<size_t>(len))uses a 64-bit length read straight from the file. A corrupt or hostile GGUF that declares a huge string length makesresizethrowstd::length_errororstd::bad_alloc. The exception escapesskippy_model_info_open, which is anextern "C"function called from the Rust FFI. That terminates the process.- Line 1582:
std::fseek(file, static_cast<long>(bytes), SEEK_CUR)narrows auint64_ttolong. On any target wherelongis 32-bit, a large skip silently seeks to the wrong offset instead of failing.- Line 1546 and Line 1549:
std::stolon a regex capture throwsstd::out_of_rangefor a long digit run in a tensor name.Bound the string length against the file size, reject oversized skips, and replace
std::stolwith a non-throwing parse.🛡️ Proposed hardening for the string read
+// A GGUF metadata string longer than this is treated as corrupt. +static constexpr uint64_t SKIPPY_GGUF_MAX_STRING_BYTES = 64ull * 1024 * 1024; + static bool skippy_read_string(FILE * file, std::string & out) { uint64_t len = 0; if (!skippy_read(file, len)) { return false; } + if (len > SKIPPY_GGUF_MAX_STRING_BYTES) { + return false; + } out.resize(static_cast<size_t>(len)); return len == 0 || std::fread(out.data(), 1, static_cast<size_t>(len), file) == len; }For the seek, reject values that do not fit the platform
long:static bool skippy_skip(FILE * file, uint64_t bytes) { + if (bytes > static_cast<uint64_t>(std::numeric_limits<long>::max())) { + return false; + } return std::fseek(file, static_cast<long>(bytes), SEEK_CUR) == 0; }🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 1576 - 1592, Harden the manual GGUF parser helpers and tensor-name parsing: update skippy_read_string to validate len against the available file size before resizing or reading, make skippy_skip reject uint64_t values that cannot fit in long before calling fseek, and replace std::stol usage in skippy_parse_tensor_metadata with a non-throwing, range-checked integer parse. Ensure malformed input returns failure rather than allowing exceptions or incorrect seeks to escape skippy_model_info_open.
155-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
skippy_env_enabledandskippy_env_disableddisagree on uppercase values.
skippy_env_enabledcompares only the lowercase forms0,false,off, andno.skippy_env_disabledalso comparesFALSE,OFF, andNO. If an operator setsSKIPPY_SOMETHING=FALSE,skippy_env_enabledreturnstrueand enables the feature. Normalize the value once and use one comparison set in both helpers.🐛 Proposed fix to normalize the value
+inline bool skippy_env_matches_false(const char * value) { + return std::strcmp(value, "0") == 0 || + std::strcasecmp(value, "false") == 0 || + std::strcasecmp(value, "off") == 0 || + std::strcasecmp(value, "no") == 0; +} + inline bool skippy_env_enabled(const char * name) { const char * value = std::getenv(name); if (value == nullptr || value[0] == '\0') { return false; } - return std::strcmp(value, "0") != 0 && - std::strcmp(value, "false") != 0 && - std::strcmp(value, "off") != 0 && - std::strcmp(value, "no") != 0; + return !skippy_env_matches_false(value); } inline bool skippy_env_disabled(const char * name) { const char * value = std::getenv(name); if (value == nullptr || value[0] == '\0') { return false; } - return std::strcmp(value, "0") == 0 || - std::strcmp(value, "false") == 0 || - std::strcmp(value, "FALSE") == 0 || - std::strcmp(value, "off") == 0 || - std::strcmp(value, "OFF") == 0 || - std::strcmp(value, "no") == 0 || - std::strcmp(value, "NO") == 0; + return skippy_env_matches_false(value); }
strcasecmpneeds<strings.h>on POSIX. Use a small local lowercase helper if you must stay portable to MSVC.🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 155 - 178, Normalize environment-variable values case-insensitively once and reuse the same comparison set in both skippy_env_enabled and skippy_env_disabled. Ensure values such as FALSE, OFF, and NO are treated consistently as disabled, using a portable local lowercase helper if needed rather than platform-specific strcasecmp.
1365-1378: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
skippy_model_attach_mtp_draft_modelcan mutate a model that has live sessions.The function checks only
target_model,target_model->ctx,path, and thatmtp_ctxis null. It then callsllama_set_embeddings_nextn(target_model->ctx, true, false)at Line 1427 and storesmtp_ctxandmtp_modelon the sharedskippy_model.In patch 0007,
skippy_session_createsetssession->ctx = model->ctxand every session shares that one context. If a caller attaches a draft model while sessions are decoding, this changes the embeddings-nextn mode under those sessions and publishes anmtp_ctxthat existing sessions did not initialize their MTP state for.skippy_modelalso has no lock coveringmtp_ctxormtp_model, so a concurrentskippy_model_freeor session read races with these writes.Reject the call when any execution lane is in use, and take
lane_mutexaround themtp_ctx/mtp_modelwrites.🛡️ Proposed guard
if (target_model->mtp_ctx != nullptr) { skippy_set_error(out_error, SKIPPY_STATUS_INVALID_ARGUMENT, "target model already has an MTP draft context"); return SKIPPY_STATUS_INVALID_ARGUMENT; } + { + std::lock_guard<std::mutex> lock(target_model->lane_mutex); + for (const bool in_use : target_model->lane_in_use) { + if (in_use) { + skippy_set_error(out_error, SKIPPY_STATUS_RUNTIME_ERROR, + "cannot attach an MTP draft model while execution lanes are in use"); + return SKIPPY_STATUS_RUNTIME_ERROR; + } + } + }
lane_mutexis added toskippy_modelin patch 0007.🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 1365 - 1378, Update skippy_model_attach_mtp_draft_model to reject attachment whenever any execution lane is active, using the model’s lane tracking and lane_mutex introduced with skippy_model. Hold lane_mutex while publishing mtp_ctx and mtp_model, and preserve the existing argument and duplicate-attachment validation.third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch (1)
1404-1430: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate the MTP sidecar state when the proposal is rejected.
When
stop_after_first_mismatchis true and rowimismatches, this code truncatessession->token_historyto the accepted prefix and skips the MTP proposal. It does not touch the sidecar state.
skippy_verify_token_batchalready ranskippy_mtp_sync_target_inputsfor the fulltoken_countwindow. That call advancedsession->mtp_next_posand overwrotesession->mtp_pending_hwith the hidden row of the last, rejected token.session->mtp_prefix_validstays true. A laterskippy_decode_step_sampled_mtpcall then reachesskippy_mtp_propose_nextwith pending hidden state derived from the rejected suffix and emits a draft from stale sidecar state.Clear the sidecar state on the rejection path.
🐛 Proposed fix
proposal_mismatched = true; *out_token_count = i + 1; + // The sidecar was synced across the full window, including the + // rejected suffix. Drop that state so later proposals cannot + // reuse hidden rows derived from rejected tokens. + skippy_mtp_clear_session_state(session); break;
skippy_mtp_clear_session_stateis declared insrc/skippy/speculative_internal.h, which this file already includes at Line 1258.🤖 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/0009-Add-Skippy-staged-execution-paths.patch` around lines 1404 - 1430, Clear the MTP sidecar state when the first proposal mismatch is detected in the loop in skippy_verify_token_batch. Invoke skippy_mtp_clear_session_state(session) alongside truncating out_token_count, while preserving the existing accepted-prefix handling and later proposal skip.Source: Learnings
third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch (1)
1462-1468: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate the sidecar prefix when a chained MTP decode fails.
This path already removed the sidecar KV range at Line 1451 and may have decoded earlier depths successfully. When
llama_decodefails, the function returnsSKIPPY_STATUS_RUNTIME_ERRORand leavessession->mtp_prefix_validtrue andsession->mtp_has_pending_htrue.
skippy_mtp_sync_target_inputssetssession->mtp_prefix_valid = falseon its own decode failure at Line 1288. This path does not. A laterskippy_mtp_propose_nextcall therefore passes the guard at Lines 1397-1400 and drafts from a sidecar cache that holds a partially written depth.The non-chained path at Lines 1510-1513 has the same gap.
🐛 Proposed fix for the chained path
if (rc != 0) { std::free(batch.token); batch.token = nullptr; llama_batch_free(batch); + session->mtp_prefix_valid = false; skippy_set_error(out_error, SKIPPY_STATUS_RUNTIME_ERROR, "llama_decode failed for chained MTP proposal"); return SKIPPY_STATUS_RUNTIME_ERROR; }🐛 Proposed fix for the non-chained path
if (rc != 0) { + session->mtp_prefix_valid = false; skippy_set_error(out_error, SKIPPY_STATUS_RUNTIME_ERROR, "llama_decode failed for MTP sidecar proposal"); return SKIPPY_STATUS_RUNTIME_ERROR; }Based on learnings: if a per-depth decode fails, invalidate
mtp_prefix_validfor that depth so later proposal generation cannot reuse previously cached or stale sidecar state.🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 1462 - 1468, Update the chained and non-chained MTP decode failure paths in the surrounding proposal-generation function to invalidate the failed depth’s sidecar prefix before returning: set the corresponding session->mtp_prefix_valid entry to false and clear session->mtp_has_pending_h as needed. Preserve the existing cleanup and runtime-error return behavior, and ensure per-depth failures cannot be reused by later skippy_mtp_propose_next calls.Source: Learnings
🧹 Nitpick comments (9)
third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch (2)
1560-1596: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe error strings say "borrowed execution lane" but
borrowed_sequenceis set to false.Lines 1569 and 1578 report "failed to clear borrowed execution lane" and "failed to trim borrowed execution lane". Line 1588 then sets
session->borrowed_sequence = false.The value is correct.
skippy_session_create_from_resident_prefixclaims the lane itself at Line 1535, soskippy_session_freemust release it at Line 658, and that release runs only whenborrowed_sequenceis false.The wording still suggests the opposite to a reader. Align the messages with the flag, or add a short comment that states the lane is owned by this session and is not borrowed.
🤖 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/0007-Add-Skippy-session-and-state-management.patch` around lines 1560 - 1596, Align the error messages in skippy_session_create_from_resident_prefix with session->borrowed_sequence = false by describing the lane as owned by the session rather than borrowed, or add a concise comment documenting that ownership. Preserve the existing flag value and release behavior.
1620-1645: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the new imported-cell-count validation.
The three tests cover KV-cell contiguity, KV-page export and import, and checkpoint retirement. The negative cases in
tests/test-skippy-kv-page-export.cppare thorough.
skippy_validate_imported_cell_countis the new logic added in response to the earlier review, and no test exercises it. Add a case that passes a state buffer whose declared cell count exceedsllama_n_ctxand assertsSKIPPY_STATUS_INVALID_ARGUMENT. That case also pins the wire-format offsets, so a future llama.cpp header change fails the test instead of corruptingn_pastsilently.I can draft that test if you want.
Also applies to: 1646-1910, 1911-1942
🤖 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/0007-Add-Skippy-session-and-state-management.patch` around lines 1620 - 1645, Extend the Skippy test coverage by adding a negative import case in the existing KV-page export/import tests that declares more imported cells than llama_n_ctx, then assert skippy_validate_imported_cell_count returns SKIPPY_STATUS_INVALID_ARGUMENT. Construct the state buffer using the expected wire-format offsets so the test also detects header layout changes; keep the existing contiguity and other test cases unchanged.third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch (3)
1171-1189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe inner
graph_filter_scopeshadows the outer one and clears the filter early.Line 1158 constructs
skippy_graph_filter_scope graph_filter_scope(config). Line 1176 constructs a second object with the same name and the same config inside a nested block. The inner object is destroyed at line 1178. Its destructor clears the graph filter while the outer object at line 1158 is still alive and still believes the filter is set.No load happens between line 1178 and the end of the function, so behavior is correct today. The shadowing is a hazard for later edits. Remove the inner scope, because the outer one already covers this range.
♻️ Proposed refactor
mtp_params.embeddings = false; - { - skippy_graph_filter_scope graph_filter_scope(config); - stage_model->mtp_ctx = llama_init_from_model(model, mtp_params); - } + stage_model->mtp_ctx = llama_init_from_model(model, mtp_params);🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 1171 - 1189, Remove the nested block and inner skippy_graph_filter_scope declaration around stage_model->mtp_ctx in the native MTP sidecar setup; reuse the outer graph_filter_scope created earlier in the surrounding function so its lifetime remains consistent through the entire operation.
957-1063: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the chained architecture comparison with a lookup table.
This condition chains about 100
model->arch != LLM_ARCH_Xterms. Every new supported architecture requires an edit inside a 100-line boolean expression. A reviewer cannot tell whether an entry is missing or duplicated.LLM_ARCH_QWEN3VLandLLM_ARCH_QWEN3VLMOEappear next toLLM_ARCH_QWEN2MOE, so the list is not ordered, which makes duplicate detection harder.Move the allow-list into a static container and test membership.
♻️ Proposed refactor
+static bool skippy_arch_supports_runtime_slice(llm_arch arch) { + static const std::unordered_set<int> supported = { + LLM_ARCH_LLAMA, + LLM_ARCH_AFMOE, + // ... one entry per line, kept sorted + LLM_ARCH_XVERSE, + }; + return supported.count(static_cast<int>(arch)) != 0; +}Then the check becomes:
- if (model->arch != LLM_ARCH_LLAMA && - /* ... ~100 more terms ... */ - model->arch != LLM_ARCH_XVERSE) { + if (!skippy_arch_supports_runtime_slice(model->arch)) {🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 957 - 1063, Replace the chained architecture comparisons in the model architecture validation condition with a static allow-list container containing the same LLM_ARCH_* values, then test model->arch membership in that container. Preserve the current supported-architecture set and rejection behavior while making entries centrally searchable and maintainable.
1240-1277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe file split copied shared code instead of extracting it. Retiring
src/skippy.cppinto per-capability files undersrc/skippy/duplicated two blocks rather than factoring them into shared helpers. Both sites now need parallel edits for any future change.
third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch#L1240-L1277: extract the shared load preparation fromskippy_model_open_implandskippy_model_open_from_parts_implinto one helper. The two functions repeat about 40 lines coveringllama_model_paramssetup, the progress callback,llama_backend_init, backend loading,skippy_apply_selected_backend_device, the device-failure event, andskippy_filter_scope. The slice-validation block at Lines 1227-1239 and Lines 1313-1325 is also identical. Pass the load step as a callable.third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch#L202-L258: move the repeated ~50-line include prologue into one internal header.model_lifecycle.cpp,model_load_config.cpp,model_loading.cpp, andsrc/skippy/session.cppin patch 0007 all repeat it, includingnlohmann/json.hpp, thesysctl/unistd.hplatform guards, and theusing jsonalias.model_lifecycle.cppholds only four delegating wrappers yet pulls in<regex>,<fstream>, and the JSON library.🤖 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/0006-Add-Skippy-model-lifecycle-and-package-support.patch` around lines 1240 - 1277, Extract the duplicated model-load preparation and slice validation from skippy_model_open_impl and skippy_model_open_from_parts_impl into shared helpers, passing the model-load operation as a callable while preserving parameter setup, callbacks, backend selection, failure events, and skippy_filter_scope behavior; apply this at third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch:1240-1277 and its corresponding sibling block. Also move the shared include prologue into one internal header for model_lifecycle.cpp, model_load_config.cpp, model_loading.cpp, and src/skippy/session.cpp at the cited 202-258 site, removing redundant JSON, platform-guard, and alias declarations and unnecessary includes from model_lifecycle.cpp.third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch (3)
1373-1375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the llama logging macros instead of
fprintf(stderr, ...).This translation unit includes
llama-impl.hat Line 1030. That header providesLLAMA_LOG_WARNandLLAMA_LOG_ERROR, which route through the callback that embedders install. A directfprintftostderrbypasses that callback and cannot be suppressed or redirected by a host application.🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 1373 - 1375, Replace the direct fprintf(stderr, ...) call in the external Inkling MTP sync failure path with the appropriate llama logging macro from llama-impl.h, preserving the existing error message and fallback text. Keep skippy_error_free(error) unchanged and use the warning or error level that matches this failure.
886-896: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse a per-session candidate buffer.
skippy_chat_sample_tokenallocates and fills astd::vector<llama_token_data>ofn_vocabentries on every sampled token. For a 150k-token vocabulary that is about 1.8 MB of allocation and initialization per sample, and verification windows call this once per row.Store the buffer on
skippy_sessionandresizeit, or keep a function-scope buffer that is cleared and refilled. The fill loop stays, but the allocation disappears from the hot path.🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 886 - 896, Update skippy_chat_sample_token to reuse a llama_token_data candidate buffer instead of constructing a new vector for every sample. Store the buffer on skippy_session and resize it to n_vocab before the existing logits fill loop, then build cur from that storage while preserving the current candidate contents and sampling behavior.
531-569: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce the token-signal computation to two vocabulary passes.
This function walks the vocabulary three times: once for the top two logits and the maximum, once for
sum_exp, and once for the entropy. The third pass callsstd::expandstd::login double precision for every token.
skippy_record_signalruns once per sampled row. For a large vocabulary and a wide verification or batch window, this adds a measurable cost to every decode step.The first and second passes can merge, because
max_logitequalstop_logit. The entropy pass can reusep = exp(logit - max_logit) / sum_expand accumulatesum_exp_x = sum(exp(l - max) * (l - max)), which yieldsentropy = log(sum_exp) - sum_exp_x / sum_expwithout a second exponential per token.Consider gating signal recording behind a flag if callers do not always consume
signal_history.🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 531 - 569, Reduce skippy_record_signal’s vocabulary scan to two passes: merge max_logit tracking with the existing top_token/second_token pass, then compute each token’s exp(logit - max_logit) once while accumulating both sum_exp and the weighted logit-difference sum needed for entropy. Derive entropy from those accumulated values instead of performing a third pass with per-token double-precision exp/log calls; only gate recording behind a flag if the surrounding callers expose a safe signal-history consumption check.third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch (1)
559-618: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe retired
src/skippy.cppinclude block is copied verbatim into both new translation units. Both files pull ingguf.h,llama-arch.h,llama-model-loader.h, the fourllama-kv-cache-*/llama-memory-*headers,../vendor/nlohmann/json.hpp,<regex>,<fstream>,<thread>,<set>, and the platformsysctl/unistdblock, plus an unusedusing json = nlohmann::ordered_json;alias. Neither file uses those facilities. The shared root cause is that the split reused the monolithic include preamble instead of deriving each file's own include set.
third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch#L559-L618: reduce the preamble ofsrc/skippy/execution-single.cppto the headers its prefill, decode, verify, and MTP wrappers use, and delete thejsonalias.third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch#L1202-L1261: apply the same reduction tosrc/skippy/verification.cpp, which needs only the session, activation, checkpoint, sampling, and speculative internal headers.🤖 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/0009-Add-Skippy-staged-execution-paths.patch` around lines 559 - 618, Reduce the copied include preambles in execution-single.cpp (lines 559-618) and verification.cpp (lines 1202-1261) to each translation unit’s actually used headers: execution-single.cpp should retain only dependencies for its prefill, decode, verify, and MTP wrappers, while verification.cpp should retain only session, activation, checkpoint, sampling, and speculative internal headers. Remove unused standard/platform includes and the using json = nlohmann::ordered_json alias from both sites.
🤖 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 `@third_party/llama.cpp/patches/0005-Add-Skippy-public-ABI-surface.patch`:
- Around line 592-616: Update skippy_abi_features() to include
SKIPPY_FEATURE_CHAT_SAMPLING_GRAMMAR in its returned feature mask, keeping the
declared and documented grammar-constrained chat sampling capability advertised
to callers.
In
`@third_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch`:
- Around line 1-4: Renumber the llama.cpp patch queue to contain exactly 11
contiguous functional patches, ensuring this staged runtime build/test change is
folded into the terminal owned patch or assigned the correct existing queue
position. Update the patch filename and its Subject line consistently,
preserving unique numbering from 0001 through 0011.
---
Outside diff comments:
In
`@third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch`:
- Around line 1809-1880: Update skippy_copy_source_tensors so every failure path
after creating output_path removes the partial output file before returning
SKIPPY_STATUS_IO_ERROR, including the initial gguf_write_to_file failure and the
later read, write, seek, or fclose failures. Reuse the existing output_path
value and preserve the current error reporting and success behavior.
- Around line 180-194: Update skippy_env_u32 to validate the parsed
unsigned-long value before converting it to uint32_t: reject inputs where
strtoul reports a negative value, clamp the wide parsed value to the uint32_t
bounds and configured min_value/max_value, then cast only after clamping; retain
fallback handling for parse and range errors.
- Around line 1194-1198: Replace the raw pointer value assigned by the
event_scope model-id path with a process-safe monotonic counter, and add the
required atomic support. Ensure each model receives a stable unique numeric
identifier without exposing stage_model’s heap address, while preserving the
existing event emission flow in the surrounding lifecycle code.
- Around line 1576-1592: Harden the manual GGUF parser helpers and tensor-name
parsing: update skippy_read_string to validate len against the available file
size before resizing or reading, make skippy_skip reject uint64_t values that
cannot fit in long before calling fseek, and replace std::stol usage in
skippy_parse_tensor_metadata with a non-throwing, range-checked integer parse.
Ensure malformed input returns failure rather than allowing exceptions or
incorrect seeks to escape skippy_model_info_open.
- Around line 155-178: Normalize environment-variable values case-insensitively
once and reuse the same comparison set in both skippy_env_enabled and
skippy_env_disabled. Ensure values such as FALSE, OFF, and NO are treated
consistently as disabled, using a portable local lowercase helper if needed
rather than platform-specific strcasecmp.
- Around line 1365-1378: Update skippy_model_attach_mtp_draft_model to reject
attachment whenever any execution lane is active, using the model’s lane
tracking and lane_mutex introduced with skippy_model. Hold lane_mutex while
publishing mtp_ctx and mtp_model, and preserve the existing argument and
duplicate-attachment validation.
In
`@third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch`:
- Around line 630-665: Update skippy_session_free to call
skippy_clear_chat_sampling unconditionally before delete session, including
preserve-prefix and null-context paths. Ensure skippy_clear_chat_sampling is
idempotent so this remains safe when skippy_session_reset has already cleared
the samplers.
- Around line 581-588: Update the exception handlers in the session
configuration flow to return a failure status instead of skippy_success after
skippy_clear_chat_sampling(session). For the std::exception handler, include
e.what() in the error reported through out_error; ensure the catch-all handler
also reports a real failure consistently with the existing non-exception failure
path.
- Around line 723-741: Align repetition_count with the signal-history window
used for the entropy and margin metrics. In the window-statistics code, derive
the repetition scan’s start and length from count and the corresponding signal
span rather than token_history.size(), while preserving safe bounds handling;
ensure all out_window fields describe the same trailing window.
In `@third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch`:
- Around line 1404-1430: Clear the MTP sidecar state when the first proposal
mismatch is detected in the loop in skippy_verify_token_batch. Invoke
skippy_mtp_clear_session_state(session) alongside truncating out_token_count,
while preserving the existing accepted-prefix handling and later proposal skip.
In
`@third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch`:
- Around line 1462-1468: Update the chained and non-chained MTP decode failure
paths in the surrounding proposal-generation function to invalidate the failed
depth’s sidecar prefix before returning: set the corresponding
session->mtp_prefix_valid entry to false and clear session->mtp_has_pending_h as
needed. Preserve the existing cleanup and runtime-error return behavior, and
ensure per-depth failures cannot be reused by later skippy_mtp_propose_next
calls.
In
`@third_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch`:
- Around line 581-585: Update the device-type switch in the GLM-DSA sparse-test
helper to select only GPU and IGPU devices; remove
GGML_BACKEND_DEVICE_TYPE_ACCEL so test selection matches
glm_dsa_backend_config_supported.
---
Nitpick comments:
In
`@third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch`:
- Around line 1171-1189: Remove the nested block and inner
skippy_graph_filter_scope declaration around stage_model->mtp_ctx in the native
MTP sidecar setup; reuse the outer graph_filter_scope created earlier in the
surrounding function so its lifetime remains consistent through the entire
operation.
- Around line 957-1063: Replace the chained architecture comparisons in the
model architecture validation condition with a static allow-list container
containing the same LLM_ARCH_* values, then test model->arch membership in that
container. Preserve the current supported-architecture set and rejection
behavior while making entries centrally searchable and maintainable.
- Around line 1240-1277: Extract the duplicated model-load preparation and slice
validation from skippy_model_open_impl and skippy_model_open_from_parts_impl
into shared helpers, passing the model-load operation as a callable while
preserving parameter setup, callbacks, backend selection, failure events, and
skippy_filter_scope behavior; apply this at
third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch:1240-1277
and its corresponding sibling block. Also move the shared include prologue into
one internal header for model_lifecycle.cpp, model_load_config.cpp,
model_loading.cpp, and src/skippy/session.cpp at the cited 202-258 site,
removing redundant JSON, platform-guard, and alias declarations and unnecessary
includes from model_lifecycle.cpp.
In
`@third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch`:
- Around line 1560-1596: Align the error messages in
skippy_session_create_from_resident_prefix with session->borrowed_sequence =
false by describing the lane as owned by the session rather than borrowed, or
add a concise comment documenting that ownership. Preserve the existing flag
value and release behavior.
- Around line 1620-1645: Extend the Skippy test coverage by adding a negative
import case in the existing KV-page export/import tests that declares more
imported cells than llama_n_ctx, then assert skippy_validate_imported_cell_count
returns SKIPPY_STATUS_INVALID_ARGUMENT. Construct the state buffer using the
expected wire-format offsets so the test also detects header layout changes;
keep the existing contiguity and other test cases unchanged.
In `@third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch`:
- Around line 559-618: Reduce the copied include preambles in
execution-single.cpp (lines 559-618) and verification.cpp (lines 1202-1261) to
each translation unit’s actually used headers: execution-single.cpp should
retain only dependencies for its prefill, decode, verify, and MTP wrappers,
while verification.cpp should retain only session, activation, checkpoint,
sampling, and speculative internal headers. Remove unused standard/platform
includes and the using json = nlohmann::ordered_json alias from both sites.
In
`@third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch`:
- Around line 1373-1375: Replace the direct fprintf(stderr, ...) call in the
external Inkling MTP sync failure path with the appropriate llama logging macro
from llama-impl.h, preserving the existing error message and fallback text. Keep
skippy_error_free(error) unchanged and use the warning or error level that
matches this failure.
- Around line 886-896: Update skippy_chat_sample_token to reuse a
llama_token_data candidate buffer instead of constructing a new vector for every
sample. Store the buffer on skippy_session and resize it to n_vocab before the
existing logits fill loop, then build cur from that storage while preserving the
current candidate contents and sampling behavior.
- Around line 531-569: Reduce skippy_record_signal’s vocabulary scan to two
passes: merge max_logit tracking with the existing top_token/second_token pass,
then compute each token’s exp(logit - max_logit) once while accumulating both
sum_exp and the weighted logit-difference sum needed for entropy. Derive entropy
from those accumulated values instead of performing a third pass with per-token
double-precision exp/log calls; only gate recording behind a flag if the
surrounding callers expose a safe signal-history consumption check.
🪄 Autofix
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: 05984c75-cac8-4754-92bf-80d82e840f8c
📒 Files selected for processing (16)
.agents/skills/llama-patch-changes/SKILL.md.agents/skills/llama-stage-patch-changes/SKILL.mdAGENTS.mddocs/design/LLAMA_STAGE_INTEGRATION_PLAN.mdthird_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patchthird_party/llama.cpp/patches/0002-Add-staged-model-graph-and-family-support.patchthird_party/llama.cpp/patches/0003-Add-GLM-DSA-backend-execution-support.patchthird_party/llama.cpp/patches/0004-Add-Inkling-model-and-multimodal-support.patchthird_party/llama.cpp/patches/0005-Add-Skippy-public-ABI-surface.patchthird_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patchthird_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patchthird_party/llama.cpp/patches/0008-Add-Skippy-activation-frame-handling.patchthird_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patchthird_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patchthird_party/llama.cpp/patches/0011-Add-Skippy-tokenization-and-stage-chat.patchthird_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch
🚧 Files skipped from review as they are similar to previous changes (4)
- AGENTS.md
- third_party/llama.cpp/patches/0001-Add-RPC-backend-tensor-transport.patch
- .agents/skills/llama-patch-changes/SKILL.md
- .agents/skills/llama-stage-patch-changes/SKILL.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch (7)
40-177: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove the Skippy n-gram adapter under
src/skippy/.
common/ngram-cache.cppnow owns the Skippy ABI object and exported functions. Keep generic n-gram logic incommon/ngram-cache.cpp. Move the adapter to a capability-owned file such assrc/skippy/ngram_cache.cpp, and list that source explicitly in the build configuration.As per coding guidelines, capability-owned Skippy implementations must be under
src/skippy/<capability>.cpp.🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 40 - 177, Move the Skippy adapter symbols, including skippy_ngram_cache and the exported skippy_ngram_cache_* functions, out of common/ngram-cache.cpp into a capability-owned src/skippy/ngram_cache.cpp file. Leave only generic n-gram logic in common/ngram-cache.cpp, and add the new adapter source explicitly to the build configuration.Source: Coding guidelines
1147-1158: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject missing MTP inputs instead of returning success.
When MTP is available and both
token_idsandinput_embeddingsare null, this condition returns success without synchronizing the sidecar. The caller then treats the external observation as successful.Return
SKIPPY_STATUS_INVALID_ARGUMENTfor missing inputs. Clear or invalidate existing MTP state before returning if stale state can remain.🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 1147 - 1158, Update skippy_mtp_sync_target_inputs so missing token_ids and input_embeddings return SKIPPY_STATUS_INVALID_ARGUMENT when MTP is available, rather than skippy_success. Preserve the no-op success behavior when skippy_mtp_available(session) is false, and clear or invalidate any existing MTP state before rejecting inputs if stale state could remain.
378-383: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
stage_modelbefore dereferencing it.The initial guards check
sessionandsession->ctx, but the next condition dereferencessession->stage_model. A session with a valid context and no stage model can crash in bothskippy_decode_tokensandskippy_verify_token_batch.Add
session->stage_model == nullptrand, where required,session->stage_model->model == nullptrto the entry validation.Also applies to: 441-446
🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 378 - 383, Update the entry validation in both skippy_decode_tokens and skippy_verify_token_batch to reject sessions with a null stage_model before dereferencing it, and reject a null stage_model->model where required by subsequent logic. Return SKIPPY_STATUS_INVALID_ARGUMENT through skippy_set_error, preserving the existing validation behavior for other invalid inputs.
480-504: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not use token ID
0as an error sentinel.
skippy_greedy_sample_contextreturns0when the model, context, or logits are unavailable. Token ID0is valid. The MTP proposal paths store this value and can mark the proposal as available.Return
LLAMA_TOKEN_NULLor return a status separately. Check the failure result before storing a proposal token.Also applies to: 1470-1473, 1515-1518
🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 480 - 504, Update skippy_greedy_sample_context and its MTP proposal call sites to use LLAMA_TOKEN_NULL, or a separate success status, for model/context/logits failures instead of token ID 0. Ensure proposal paths around the referenced sampling logic check the failure result before storing or marking a proposal token as available, while preserving valid token ID 0.
1097-1113: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear external Inkling state when clearing MTP state.
skippy_external_decode_observestores external embeddings before synchronization. On failure, it callsskippy_mtp_clear_session_state, but that function leavesexternal_mtp_embeddings_availableandexternal_mtp_embeddingsunchanged. A later consumer can observe stale embeddings.Clear both fields in
skippy_mtp_clear_session_state.Based on learnings, MTP failure paths must clear or invalidate per-sidecar and per-depth state before later proposal generation.
Also applies to: 1373-1378
🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 1097 - 1113, Update skippy_mtp_clear_session_state to also invalidate external Inkling state by setting external_mtp_embeddings_available to false and clearing external_mtp_embeddings. Apply the same reset in the corresponding MTP failure-path implementation referenced near the additional occurrence, ensuring no stale external embeddings remain available after synchronization failure.Source: Learnings
1331-1337: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate MTP state on every post-decode failure.
The
h_last == nullptrpath returns after sidecar work without clearing the sidecar state. Both proposal decode failure paths also return whilemtp_prefix_valid, pending hidden state, or sidecar memory can still describe a partial decode. A later proposal can reuse stale state.Clear the sidecar state and invalidate the affected prefix before each return. Do not leave a failed proposal eligible for reuse.
Based on learnings, MTP failure paths must clear or invalidate per-sidecar and per-depth state before later proposal generation.
Also applies to: 1462-1467, 1510-1512
🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 1331 - 1337, Update every post-decode failure path in the MTP proposal flow, including the h_last == nullptr branch and both proposal decode failure branches, to clear pending hidden state and sidecar memory, invalidate mtp_prefix_valid, and reset affected per-sidecar/per-depth state before returning. Ensure no failed proposal remains eligible for reuse by later proposal generation.Source: Learnings
60-98: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTranslate allocation failures at the ngram cache ABI boundary.
skippy_ngram_cache_create,skippy_ngram_cache_reset,skippy_ngram_cache_append, andskippy_ngram_cache_draftcallnew,std::vector::assign,std::vector::insert, andstd::vectorcopy paths without exception handling. Ifstd::bad_allocorstd::length_errorescapes these exported C functions, callers receive no status and the process can terminate. Wrap the allocation/cache-update paths and returnSKIPPY_STATUS_RUNTIME_ERROR, using the providedout_errorwhen available.🤖 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/0010-Add-Skippy-sampling-and-speculative-decoding.patch` around lines 60 - 98, Wrap allocation- and cache-update operations in skippy_ngram_cache_create, skippy_ngram_cache_reset, skippy_ngram_cache_append, and skippy_ngram_cache_draft with exception handling for std::bad_alloc and std::length_error. Translate caught failures to SKIPPY_STATUS_RUNTIME_ERROR and populate out_error when provided, ensuring no C++ exception escapes these exported ABI functions.
🤖 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.
Outside diff comments:
In
`@third_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patch`:
- Around line 40-177: Move the Skippy adapter symbols, including
skippy_ngram_cache and the exported skippy_ngram_cache_* functions, out of
common/ngram-cache.cpp into a capability-owned src/skippy/ngram_cache.cpp file.
Leave only generic n-gram logic in common/ngram-cache.cpp, and add the new
adapter source explicitly to the build configuration.
- Around line 1147-1158: Update skippy_mtp_sync_target_inputs so missing
token_ids and input_embeddings return SKIPPY_STATUS_INVALID_ARGUMENT when MTP is
available, rather than skippy_success. Preserve the no-op success behavior when
skippy_mtp_available(session) is false, and clear or invalidate any existing MTP
state before rejecting inputs if stale state could remain.
- Around line 378-383: Update the entry validation in both skippy_decode_tokens
and skippy_verify_token_batch to reject sessions with a null stage_model before
dereferencing it, and reject a null stage_model->model where required by
subsequent logic. Return SKIPPY_STATUS_INVALID_ARGUMENT through
skippy_set_error, preserving the existing validation behavior for other invalid
inputs.
- Around line 480-504: Update skippy_greedy_sample_context and its MTP proposal
call sites to use LLAMA_TOKEN_NULL, or a separate success status, for
model/context/logits failures instead of token ID 0. Ensure proposal paths
around the referenced sampling logic check the failure result before storing or
marking a proposal token as available, while preserving valid token ID 0.
- Around line 1097-1113: Update skippy_mtp_clear_session_state to also
invalidate external Inkling state by setting external_mtp_embeddings_available
to false and clearing external_mtp_embeddings. Apply the same reset in the
corresponding MTP failure-path implementation referenced near the additional
occurrence, ensuring no stale external embeddings remain available after
synchronization failure.
- Around line 1331-1337: Update every post-decode failure path in the MTP
proposal flow, including the h_last == nullptr branch and both proposal decode
failure branches, to clear pending hidden state and sidecar memory, invalidate
mtp_prefix_valid, and reset affected per-sidecar/per-depth state before
returning. Ensure no failed proposal remains eligible for reuse by later
proposal generation.
- Around line 60-98: Wrap allocation- and cache-update operations in
skippy_ngram_cache_create, skippy_ngram_cache_reset, skippy_ngram_cache_append,
and skippy_ngram_cache_draft with exception handling for std::bad_alloc and
std::length_error. Translate caught failures to SKIPPY_STATUS_RUNTIME_ERROR and
populate out_error when provided, ensuring no C++ exception escapes these
exported ABI functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2931a28b-f225-4893-bce3-681b86f3b1b0
📒 Files selected for processing (8)
third_party/llama.cpp/patches/0005-Add-Skippy-public-ABI-surface.patchthird_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patchthird_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patchthird_party/llama.cpp/patches/0008-Add-Skippy-activation-frame-handling.patchthird_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patchthird_party/llama.cpp/patches/0010-Add-Skippy-sampling-and-speculative-decoding.patchthird_party/llama.cpp/patches/0011-Add-Skippy-tokenization-and-stage-chat.patchthird_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch
🚧 Files skipped from review as they are similar to previous changes (5)
- third_party/llama.cpp/patches/0012-Wire-staged-runtime-builds-and-tests.patch
- third_party/llama.cpp/patches/0005-Add-Skippy-public-ABI-surface.patch
- third_party/llama.cpp/patches/0009-Add-Skippy-staged-execution-paths.patch
- third_party/llama.cpp/patches/0007-Add-Skippy-session-and-state-management.patch
- third_party/llama.cpp/patches/0006-Add-Skippy-model-lifecycle-and-package-support.patch
|
Merge after minor release |
edab600 to
c3a297e
Compare
c3a297e to
618e785
Compare
618e785 to
a0cd0f5
Compare
2 participants
Summary
0066-Split-Skippy-by-functional-boundary.patch; each recreated patch now introduces and edits capability-owned files directlysrc/skippy.cppand split native implementation ownership acrosssrc/skippy/; every implementation file is below 1,000 linesinclude/skippy.has an umbrella over standalone C-compatible capability headers underinclude/skippy/Source compatibility is intentionally not preserved. Exported symbol signatures, struct layouts, and Skippy ABI version constants are unchanged.
Validation
third_party/llama.cpp/upstream.txtskippy.hand everyskippy/*.hheader as C11 and C++17 with warnings deniedcargo fmt --all --checkcargo check -p mesh-llmcargo test -p skippy-runtime --lib(69 passed)cargo test -p skippy-server --lib(382 passed)cargo test -p mesh-llm --lib(44 passed)Summary by CodeRabbit
New Features
Documentation
Tests