fix(skippy): isolate chat grammar during speculative verification - #1172
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:
📝 WalkthroughWalkthroughChangesSpeculative verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NativeMTPVerification
participant skippy_verify_tokens_frame_sampled
participant GrammarSampler
participant MTPProposalGeneration
NativeMTPVerification->>skippy_verify_tokens_frame_sampled: verify proposal tokens
skippy_verify_tokens_frame_sampled->>GrammarSampler: compare sampled tokens
GrammarSampler-->>skippy_verify_tokens_frame_sampled: first mismatch or full acceptance
skippy_verify_tokens_frame_sampled-->>NativeMTPVerification: return authoritative prediction prefix
skippy_verify_tokens_frame_sampled-->>MTPProposalGeneration: generate proposals only after full match
Possibly related PRs
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/skippy-runtime/src/tests.rs`:
- Around line 296-305: Update the serial verification loop surrounding
verify_tokens_sampled to stop immediately after the first prediction differs
from the next proposed token, rather than decoding the rejected suffix. Use that
truncated serial result as the canonical prefix for comparing
batched_predictions and the corresponding positions, preserving the existing
first-mismatch boundary.
🪄 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: 8e4bde22-e2a7-4dad-bdd1-f05cc468ef4a
📒 Files selected for processing (4)
crates/skippy-runtime/src/tests.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/native_mtp/hybrid.rsthird_party/llama.cpp/patches/0066-Isolate-chat-grammar-during-speculative-verification.patch
There was a problem hiding this comment.
I think it would be worth adding one more model-backed regression check for tool calling with a long resident KV context (8k+ tokens)... The current tests validate the short verification window, but the proposed test would sample tokens from the existing (potentially large) KV cache.
Adding a fallback check for latency and native-MTP acceptance at a sizeable context length would guard against a long-context throughput regression.
|
@ndizazzo coming right up |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/skippy-runtime/src/tests.rs (3)
454-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport when the model returns no MTP draft.
The acceptance checks run only when
draftisSome. If the model returns no draft, the test passes without exercising draft acceptance. The test name states that it verifies native MTP acceptance, so a silent pass hides the coverage gap.Log the skip, or fail if the configured model is expected to produce a draft.
♻️ Proposed change
if let Some(draft) = draft { let mut target = model.create_session_from_resident_prefix(RESIDENT_PREFIX_ID, prompt_prefix)?; @@ draft.token_ids ); + } else { + eprintln!( + "native MTP returned no draft after the resident tool context; \ + draft-acceptance coverage was skipped" + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-runtime/src/tests.rs` around lines 454 - 482, Update the native MTP acceptance test around the draft match so a missing draft is explicitly reported instead of silently skipping the acceptance assertions. Handle the None branch with a clear diagnostic, or fail the test when the configured model is expected to produce a draft, while preserving the existing checks for Some(draft).
373-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the prompt against the verify window, not only the context size.
Line 374 allows a prompt of
CONTEXT_SIZE - 1tokens. The test then decodesverify_inputs(about 20 tokens) plus 4 MTP draft tokens on top of the prefix. If the growth loop overshoots, the failure surfaces as an opaque decode error instead of a clear assertion.Assert the remaining headroom explicitly.
♻️ Proposed assertion
+ // Leave room for the verification window and MTP drafts. + const VERIFY_HEADROOM: usize = 128; assert!( - prompt_tokens.len() < CONTEXT_SIZE as usize, - "resident prompt must leave room for tool sampling" + prompt_tokens.len() + VERIFY_HEADROOM <= CONTEXT_SIZE as usize, + "resident prompt must leave room for tool sampling: {} tokens with context {CONTEXT_SIZE}", + prompt_tokens.len() );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-runtime/src/tests.rs` around lines 373 - 376, Update the prompt-size assertion in the relevant test to bound resident prompt tokens by the available context after reserving space for verify_inputs and the four MTP draft tokens, rather than only checking against CONTEXT_SIZE. Make the assertion explicitly validate sufficient remaining headroom so oversized growth fails with a clear message before verify_inputs decoding.
408-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the first-mismatch serial loop into a shared helper.
This loop is identical to the loop at Lines 292-299. The first-mismatch boundary is a contract shared by both tests. Duplicating it lets the two copies diverge.
Extract one helper and call it from both tests.
♻️ Proposed helper
fn serial_predictions_until_mismatch( session: &mut StageSession, verify_inputs: &[i32], sampling: &SamplingConfig, ) -> anyhow::Result<Vec<i32>> { let mut predictions = Vec::with_capacity(verify_inputs.len()); for (index, token) in verify_inputs.iter().copied().enumerate() { let predicted = session.decode_step_sampled(token, Some(sampling))?; predictions.push(predicted); if index + 1 < verify_inputs.len() && predicted != verify_inputs[index + 1] { break; } } Ok(predictions) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-runtime/src/tests.rs` around lines 408 - 415, Extract the duplicated first-mismatch loop into a shared `serial_predictions_until_mismatch` helper that accepts the session, verification inputs, and sampling configuration, performs the existing prediction and boundary logic, and returns `anyhow::Result<Vec<i32>>`. Replace both serial prediction loops, including the one near `Lines 292-299` and the loop in the current test, with calls to this helper while preserving their existing error propagation and behavior.
🤖 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.
Nitpick comments:
In `@crates/skippy-runtime/src/tests.rs`:
- Around line 454-482: Update the native MTP acceptance test around the draft
match so a missing draft is explicitly reported instead of silently skipping the
acceptance assertions. Handle the None branch with a clear diagnostic, or fail
the test when the configured model is expected to produce a draft, while
preserving the existing checks for Some(draft).
- Around line 373-376: Update the prompt-size assertion in the relevant test to
bound resident prompt tokens by the available context after reserving space for
verify_inputs and the four MTP draft tokens, rather than only checking against
CONTEXT_SIZE. Make the assertion explicitly validate sufficient remaining
headroom so oversized growth fails with a clear message before verify_inputs
decoding.
- Around line 408-415: Extract the duplicated first-mismatch loop into a shared
`serial_predictions_until_mismatch` helper that accepts the session,
verification inputs, and sampling configuration, performs the existing
prediction and boundary logic, and returns `anyhow::Result<Vec<i32>>`. Replace
both serial prediction loops, including the one near `Lines 292-299` and the
loop in the current test, with calls to this helper while preserving their
existing error propagation and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb38a816-fa07-4c3d-b86f-1fb291283f32
📒 Files selected for processing (1)
crates/skippy-runtime/src/tests.rs
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 (1)
third_party/llama.cpp/patches/0066-Stop-chat-grammar-verification-at-the-first-mismatch.patch (1)
34-34: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClear MTP state on grammar mismatch.
When
proposal_mismatchedstops proposal generation, an existing draft can still be returned inout_mtp_draft. Setout_mtp_draft->available = falseand clear session MTP state, for example withskippy_mtp_clear_session_state(session), before returning from this mismatch 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/0066-Stop-chat-grammar-verification-at-the-first-mismatch.patch` at line 34, Update the proposal-mismatch path guarded by proposal_mismatched so any existing out_mtp_draft is marked unavailable and the session MTP state is cleared via skippy_mtp_clear_session_state(session) before returning. Keep normal draft handling unchanged when no grammar mismatch occurs.Source: Learnings
🤖 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/0066-Stop-chat-grammar-verification-at-the-first-mismatch.patch`:
- Line 34: Update the proposal-mismatch path guarded by proposal_mismatched so
any existing out_mtp_draft is marked unavailable and the session MTP state is
cleared via skippy_mtp_clear_session_state(session) before returning. Keep
normal draft handling unchanged when no grammar mismatch occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 11057db4-5123-477a-86d1-97cdab7dfaee
📒 Files selected for processing (1)
third_party/llama.cpp/patches/0066-Stop-chat-grammar-verification-at-the-first-mismatch.patch
3 participants
Why this matters
While testing a possible shortcut, Mesh could accidentally change the rules used to produce valid tool calls. Even when that shortcut was rejected, later generation could produce malformed output.
This change makes speculative verification a safe dry run. Rejected candidate tokens cannot damage the real grammar state used by normal generation.
Technical details
Non-grammar and ordinary generation paths are unchanged.
Validation
bash scripts/prepare-llama.sh— complete 66-patch queue applied successfullycargo test -p skippy-runtime --quiet— 68 passedcargo test -p skippy-server verify_window_— 10 passedThe optional model-backed lazy-grammar correctness test compiled but skipped because
SKIPPY_CORRECTNESS_MODELwas not configured locally.This PR is based directly on
mainand has no dependency on the companion fixes.Summary by CodeRabbit