feat(skippy): expose authoritative generation lifecycle events - #1149
Conversation
📝 WalkthroughWalkthroughSkippy loading now supports tokenizer-aware serving hooks and native serving plugins. OpenAI requests propagate agent-session identities. Generation receipts track lifecycle events and timing. Linear proposal queries use validated position metadata. ChangesServing hooks and native plugin integration
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAiFrontend
participant StageOpenAiBackend
participant GenerationReceiptIngress
participant NativeServingPluginHost
participant NativeServingPluginV1
OpenAiFrontend->>StageOpenAiBackend: dispatch request with agent session
StageOpenAiBackend->>GenerationReceiptIngress: submit lifecycle observations
GenerationReceiptIngress->>NativeServingPluginHost: queue lifecycle or proposal event
NativeServingPluginHost->>NativeServingPluginV1: invoke ABI callback
NativeServingPluginV1-->>NativeServingPluginHost: return lifecycle or proposal result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
18f90ff to
3b40df4
Compare
3b40df4 to
608ea63
Compare
|
@i386 seems like you touched on this before I could get to it, but I have a more substantial event-hook lifecycle system planned. The work I have planned converts native llama.cpp and Rust runtime observations into validated, typed events instead of relying on parsed log text, and adds missing hooks for lifecycle (as this PR does). A bounded Rust event pipeline reduces those into backend-neutral lifecycle and availability state for the CLI, TUI, API, telemetry, and diagnostics.... wires up to literally everything and anything that needs it. Native execution remains authoritative, while event consumers stay nonblocking, privacy-safe, and unable to control inference. I opened up an issue #1167 that outlines the plan at a high level, and it'd be rad if you could have an agent shape this specific PR in a way that would make it integrate with the plan smoothly |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
crates/skippy-server/src/lib.rs (1)
32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove new crate-root forwarding re-exports.
These exports add public paths that do not identify the owning subsystem. Export the APIs from their owning
skippy_servermodules and import them directly at consumers.
crates/skippy-server/src/lib.rs#L32-L42: expose lifecycle types fromfrontendand serving-hook types from a public owning module instead of new crate-root re-exports.crates/mesh-llm-host-runtime/src/lib.rs#L40-L40: remove the forwarding exports and import theskippy_serverowning-module types where the host runtime uses them.As per coding guidelines: “Minimize crate-root re-exports. Temporary compatibility re-exports are allowed during refactors, but new code should import from the owning module directly and transitional re-exports should be removed afterward.” As per coding guidelines: “Keep the host-runtime
lib.rsslim and use it as an entry point rather than a general-purpose code container.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/lib.rs` around lines 32 - 42, Remove the new crate-root forwarding re-exports from crates/skippy-server/src/lib.rs lines 32-42, exposing lifecycle APIs through frontend and serving-hook APIs through their public owning module instead. In crates/mesh-llm-host-runtime/src/lib.rs line 40, remove the forwarding exports and update host-runtime consumers to import those types directly from the corresponding skippy_server owning modules; keep both crate-root entry points slim.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh-llm-host-runtime/src/runtime/local_split/loading.rs`:
- Line 199: Update SplitGenerationLoadSpec and the split stage-0 loading path in
load_stage0_runtime_options_with_openai_args_and_open_events to carry and pass
the serving hooks factory instead of None, matching
start_local_layer_package_model; if split serving intentionally excludes
lifecycle hooks, explicitly document that contract and add coverage for the
hook-free stage-0 behavior.
In `@crates/skippy-server/src/frontend/generation_flow.rs`:
- Around line 447-459: Ensure split multimodal generations also emit the
complete lifecycle events: begin, record, committed, and abort. Update the
shared generation wrapper around the current GenerationStart emission, or add
equivalent receipt observation and finalization within
generate_split_multimodal_text, while preserving existing behavior for non-split
generations.
In `@crates/skippy-server/src/frontend/generation_receipt.rs`:
- Around line 97-111: The GenerationReceiptSink callbacks currently run
synchronously on the generation path and propagate observer failures as
OpenAiError. Update the call sites around local_generation and generation_flow
to enqueue lifecycle events through a bounded, nonblocking delivery mechanism
that preserves per-request ordering, and report queue or callback failures via
telemetry or explicit delivery state rather than model execution errors. Adjust
GenerationReceiptSink integration as needed while preserving the
begin/committed/record-or-abort lifecycle.
In `@crates/skippy-server/src/frontend/linear_proposal.rs`:
- Around line 804-829: Extract the entire #[cfg(test)] mod tests from the linear
proposal implementation into a sibling module named linear_proposal_tests.rs,
including FakeIngress and all related fixtures and test helpers. Leave only the
test-module declaration in the original file, and adjust visibility/imports so
the moved tests continue to exercise the same behavior while keeping the new
module under 1,000 lines.
In `@crates/skippy-server/src/frontend/local_generation.rs`:
- Around line 1124-1133: Update abort_after_failed_receipt so a record error
does not trigger config.sink().abort, since the receipt may already have been
persisted; preserve the receipt_result return value while preventing both
terminal events from being emitted for one begun request/session. Align the
terminal delivery behavior with GenerationReceiptSink’s acknowledged outcome
contract, using durable idempotency or outcome sequencing if needed.
In `@crates/skippy-server/src/serving_hooks.rs`:
- Around line 28-46: Update ModelServingHooks::new and the surrounding API to
allow configuring generation_receipt and linear_proposal_ingress independently,
adding builder methods or single-purpose constructors while preserving the
existing both-hooks convenience path. Ensure callers can enable either hook
without supplying a dummy configuration, and retain the optional getter
behavior.
---
Nitpick comments:
In `@crates/skippy-server/src/lib.rs`:
- Around line 32-42: Remove the new crate-root forwarding re-exports from
crates/skippy-server/src/lib.rs lines 32-42, exposing lifecycle APIs through
frontend and serving-hook APIs through their public owning module instead. In
crates/mesh-llm-host-runtime/src/lib.rs line 40, remove the forwarding exports
and update host-runtime consumers to import those types directly from the
corresponding skippy_server owning modules; keep both crate-root entry points
slim.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 30f81668-4b8a-4de2-9a5c-fd5b89a5c009
📒 Files selected for processing (13)
crates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/lib.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-host-runtime/src/runtime/local_model_only.rscrates/mesh-llm-host-runtime/src/runtime/local_split/loading.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/generation/cache_hints.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/generation_receipt.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/lib.rscrates/skippy-server/src/serving_hooks.rs
18aeb33 to
5303997
Compare
* feat(openai): propagate stable agent session identity * feat(mesh): load deadline-safe native serving plugins * docs: document native serving integrations * fix(skippy): preserve native proposal query contract * fix(skippy): compile native proposal plugin tests * fix(plugin): use public proposal query constructor
e979b9f to
7ef248f
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh-llm-host-runtime/src/runtime/local_split/loading.rs (1)
342-352: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBudget timeout from selected transfer artifacts, not uniform layer weights.
stage_source_prepare_timeoutestimates layer-package timeouts assource_model_bytes / layer_count * stage_layers, but artifact transfer fetchesrequired_stage_package_artifacts(...)forlayer_start..layer_end, plus manifest/embeddings/outputs/projectors when selected. A stage that covers the heavy manifest or first/last artifacts can exceed the configured transfer rate and hit this timeout. Pass selected artifact byte sizes to the timeout formula, or derive it from the artifact selection so uneven packages do not under-budget.🤖 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/mesh-llm-host-runtime/src/runtime/local_split/loading.rs` around lines 342 - 352, Update stage_source_prepare_timeout to calculate the transfer budget from the byte sizes of the artifacts selected by required_stage_package_artifacts for the stage range, including optional manifest, embeddings, outputs, and projectors when selected, rather than distributing source_model_bytes uniformly across layers. Use the summed selected-artifact bytes with the existing transfer-rate, allowance, and minimum-timeout logic, and adjust callers to provide or derive that selection.
🧹 Nitpick comments (10)
crates/mesh-native-serving-plugin-host/src/lib.rs (1)
701-712: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTie the returned
&strlifetime to the caller-provided data.
read_utf8<'a>produces an unbounded lifetime from a raw pointer. The compiler cannot check that the plugin table outlives the returned reference. The single current caller converts toStringimmediately, so behavior is correct today, but the signature invites an unsound use later.Return
String, or accept a lifetime-carrying argument that anchors'a.🤖 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/mesh-native-serving-plugin-host/src/lib.rs` around lines 701 - 712, Update read_utf8 so it no longer returns an unbounded borrowed &str from the raw ByteSlice; return an owned String instead, preserving the existing null-pointer validation and UTF-8 error context, and adjust its sole caller to use the owned value while retaining the current behavior.crates/mesh-llm-cli/src/parser/commands.rs (1)
501-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReject a zero deadline during parsing.
NativeServingPluginFactory::loadrejects a zero proposal deadline at runtime. Avalue_parserrange moves that failure to argument parsing and gives a clearer message.♻️ Proposed refactor
/// Mesh-enforced hard proposal deadline in milliseconds. - #[arg(long, hide = true, requires = "native_serving_plugin")] + #[arg( + long, + hide = true, + requires = "native_serving_plugin", + value_parser = clap::value_parser!(u64).range(1..) + )] pub native_serving_plugin_deadline_ms: Option<u64>,🤖 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/mesh-llm-cli/src/parser/commands.rs` around lines 501 - 503, Update the native_serving_plugin_deadline_ms argument definition in the CLI command parser to use a value_parser that accepts only positive u64 values, rejecting zero during argument parsing while preserving the existing optional and dependency behavior.crates/mesh-native-serving-plugin-api/src/lib.rs (1)
152-204: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an explicit extension rule for the event structs.
ActivationContextandNativeServingPluginV1both carrystruct_size, so the host can validate their layout. The lifecycle and proposal event structs carry no size field. A later additive field inGenerationFinishorProposalQuerytherefore changes the layout with no way for a v1 plugin to detect it, and the only remaining option is a full ABI-version bump.Either add a leading
struct_sizeto each event struct, or document that every layout change requires a newNATIVE_SERVING_PLUGIN_ABI_V*table so v1 plugins keep working.As per coding guidelines: "When changing the plugin protocol, preserve support for the previous version unless a breaking change is explicitly intended and confirmed."
♻️ Example: size-prefixed event structs
#[repr(C)] #[derive(Clone, Copy, Debug, Default)] pub struct GenerationStart { + pub struct_size: usize, pub request_id: u64, pub session_id: u64, pub agent_session_id: ByteSlice, pub prompt_token_ids: TokenSlice, }🤖 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/mesh-native-serving-plugin-api/src/lib.rs` around lines 152 - 204, Add an explicit ABI extension strategy for the lifecycle and proposal event structs shown here: preferably add a leading struct_size field to GenerationStart, GenerationCommit, GenerationAbort, GenerationFinish, and ProposalQuery, matching the existing size-validation approach used by ActivationContext and NativeServingPluginV1. Ensure the host can validate truncated/older layouts so additive fields remain compatible; otherwise document and implement a new ABI table for every layout change.Source: Coding guidelines
crates/openai-frontend/src/completions.rs (1)
47-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated agent-session metadata helpers in two request types.
ChatCompletionRequestandCompletionRequesteach define the same two internal key constants and the same three methods. The shared root cause is the absence of one helper incommon.rs. Two copies of the key names can drift, and drift would silently break the strip-then-set protection against body-supplied session keys.
crates/openai-frontend/src/completions.rs#L47-L74: replaceset_agent_session,agent_session, andagent_session_sourcewith calls to the shared helper incommon.rs, and delete the local key constants at Lines 14-15.crates/openai-frontend/src/chat.rs#L51-L78: apply the same replacement, and delete the local key constants at Lines 14-15.🤖 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/openai-frontend/src/completions.rs` around lines 47 - 74, Centralize the duplicated agent-session metadata logic by adding or reusing one shared helper in common.rs. In crates/openai-frontend/src/completions.rs lines 47-74 and crates/openai-frontend/src/chat.rs lines 51-78, replace the local set_agent_session, agent_session, and agent_session_source implementations with the shared helper, and remove each file’s local agent-session key constants at lines 14-15; preserve the strip-then-set behavior.crates/openai-frontend/src/responses.rs (1)
552-586: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture ordering is correct.
The
conversationread at Lines 563-565 runs beforetranslate_openai_responses_inputremoves the field at Line 570, so the identity survives normalization.One naming point:
responses_conversation_cache_keynow serves two purposes, the prompt-cache key and the agent-session identity. Rename it toresponses_conversation_idso both call sites read correctly. The doc comment onAgentSessionSourceincrates/openai-frontend/src/common.rsLines 13-15 states the identity is not a prompt-cache key, while both values now derive from the same helper. Update that comment or the helper name.🤖 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/openai-frontend/src/responses.rs` around lines 552 - 586, Rename the shared helper responses_conversation_cache_key to responses_conversation_id and update both its prompt-cache and agent-session call sites, including the usage in the normalization flow around translate_openai_responses_input. Revise the AgentSessionSource documentation in common.rs to reflect that the conversation ID is used as agent-session identity and may also derive the prompt-cache key, without changing the existing capture ordering.crates/skippy-server/src/frontend/generation_receipt.rs (1)
876-901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the source-text ordering test with a behavioral assertion.
This test reads
local_generation/token_generation.rswithinclude_str!and asserts on the byte offsets of literal source fragments. Renaming a local variable or reformatting a call breaks the test without any behavior change. The fragment at Line 886 already encodes an exact expression, including the?operator.Assert the ordering through a recording
GenerationLifecycleIngressinstead. Drive one generation, then assert that the observed sequence isStarted,Committed, and a terminalCompletedorAborted, and that cleanup ran last.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/generation_receipt.rs` around lines 876 - 901, Replace receipt_lifecycle_begins_before_generation_and_closes_before_cleanup’s include_str!/source-offset checks with a behavioral test using a recording GenerationLifecycleIngress. Drive one local generation and record lifecycle events, then assert the sequence contains Started, Committed, and a terminal Completed or Aborted event, with cleanup observed last; avoid asserting source text or exact local expressions.crates/skippy-server/src/lib.rs (1)
32-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the re-export surface for the two
ModelServingHooksparameter types.
ModelServingHooks::newtakesGenerationReceiptConfigandLinearProposalIngressConfig. After this change, Line 33 still re-exportsLinearProposalIngressConfigfrom the crate root, butGenerationReceiptConfigis reachable only throughfrontend. An external caller must therefore writeskippy_server::LinearProposalIngressConfigandskippy_server::frontend::GenerationReceiptConfigin the same call.Pick one convention for both types. The coding guidelines favor importing from the owning module, so removing
LinearProposalIngressConfigfrom this list is consistent with the receipt-type removal.As per coding guidelines: "Minimize crate-root re-exports. Temporary compatibility re-exports are allowed during refactors, but new code should import from the owning module directly and transitional re-exports should be removed afterward."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/lib.rs` around lines 32 - 37, Remove LinearProposalIngressConfig from the crate-root re-export list in lib.rs, matching GenerationReceiptConfig’s module-owned access through frontend. Keep the type available via its owning module so ModelServingHooks::new callers use consistent import paths.Source: Coding guidelines
crates/skippy-server/src/frontend/local_generation/token_generation.rs (1)
94-101: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider sharing the prompt token IDs instead of copying them twice.
Line 99 copies the prompt into a
Box<[i32]>forGenerationStart.build_generation_receiptincrates/skippy-server/src/frontend/generation_receipt.rs, Line 455, copies the same prompt again for the receipt. Each configured request therefore performs two prompt-sized allocations on the generation path.Storing the prompt as
Arc<[i32]>inGenerationStartandGenerationReceiptremoves both copies and keeps the exact token evidence. This is a public type change, so apply it only if the plugin ABI conversion incrates/mesh-native-serving-plugin-host/src/lib.rsstays unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/local_generation/token_generation.rs` around lines 94 - 101, Change prompt token storage in GenerationStart and GenerationReceipt to Arc<[i32]> so the existing prompt allocation can be shared instead of copied by both receipt paths. Update the construction and propagation in the surrounding generation flow, including build_generation_receipt, while preserving the exact token evidence; verify the plugin ABI conversion in lib.rs remains unchanged.crates/skippy-server/src/frontend/generation_flow.rs (1)
636-640: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTwo modified Rust files exceed the 1,000-line extraction threshold. This PR modifies both files, and each is already over 1,000 lines. The coding guidelines require extracting a separable responsibility into a semantically named module and keeping the new file under 1,000 lines. Both files contain a clearly separable flow that can move without changing behavior.
crates/skippy-server/src/frontend/generation_flow.rs#L636-L640: extractgenerate_split_multimodal_textinto a module such asgeneration_flow/split_multimodal.rs, and move its tests with it.crates/skippy-server/src/frontend/local_generation/token_generation.rs#L84-L101: extract the linear-proposal decode branch, for exampletry_execute_linear_proposaland its helpers, into a module such aslocal_generation/linear_decode.rs, and move its tests with it.As per coding guidelines: "When modifying a Rust source file over 1,000 lines, extract any separable responsibility into a semantically named module, keep the new file under 1,000 lines, and move relevant tests with the extracted behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/generation_flow.rs` around lines 636 - 640, Extract the split multimodal generation responsibility centered on generate_split_multimodal_text from crates/skippy-server/src/frontend/generation_flow.rs:636-640 into a semantically named module such as generation_flow/split_multimodal.rs, moving its related tests and preserving behavior; separately extract try_execute_linear_proposal and its helpers from crates/skippy-server/src/frontend/local_generation/token_generation.rs:84-101 into a module such as local_generation/linear_decode.rs, moving the associated tests. Ensure both original files are reduced appropriately and each new module remains under 1,000 lines.Source: Coding guidelines
crates/skippy-server/src/frontend/local_generation/tests.rs (1)
208-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the sink failure is accounted.
This phase sets
sink.failand then verifies that session cleanup still happened. It does not verify the new accounting path. The sink pushes the receipt before it fails, sowait_for_receipts(&sink, 2)succeeds even if the worker never records the delivery failure.Add an assertion on
GenerationReceiptConfig::delivery_failuresfor the configured receipt config. That covers the asynchronous failure counter introduced in this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/local_generation/tests.rs` around lines 208 - 234, Extend the failing-sink test around `sink.fail` and `failing_ids` to assert that the configured `GenerationReceiptConfig::delivery_failures` counter increments after the failed receipt is processed. Keep the existing session cleanup assertion, and ensure the check occurs after waiting for asynchronous receipt handling.
🤖 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/mesh-llm-host-runtime/src/runtime/options.rs`:
- Around line 40-43: Wire the native serving plugin options into the local
startup path used by start_runtime_local_model, ensuring the configured plugin,
config, state, and deadline reach the serving hooks factory instead of always
passing None; if this path cannot consume them, remove the corresponding runtime
option fields and CLI handling.
In `@crates/mesh-native-serving-plugin-host/src/lib.rs`:
- Around line 633-676: Update convert_termination, convert_disposition, and
convert_discard_reason to add explicit mappings for all currently defined source
enum variants before their wildcard arms. Preserve the existing fallback
behavior only for genuinely unknown future variants, ensuring newly supported
authoritative outcomes map to their corresponding ABI values rather than
defaulting to CANCELLED, STOPPED, or EXECUTION_FAILED.
- Around line 124-127: Update the host initialization around
ModelServingHooks::new and LinearProposalIngressConfig::new to use an explicit,
appropriate proposal-token cap instead of usize::MAX, then modify
poll_until_deadline to allocate the token buffer once and reuse it across
polling iterations. Preserve existing proposal decoding behavior while
preventing per-proposal allocations sized directly by an unbounded request.
- Around line 333-345: Update the failure handling in the proposal polling flow:
call cancel_proposal for the current decision_id before returning an error from
the ProposalPollStatus::FAILED branch and when proposal_from_output rejects in
the READY branch. Preserve the existing status_error and parsing errors, and
ensure cancellation occurs before either error is returned.
- Around line 594-601: Update the error handling around the worker’s command
result and ensure_healthy so non-terminal lifecycle event failures do not
populate fatal_error or permanently reject later enqueue/propose calls. Keep
lifecycle_delivery_failures as the accounting mechanism for recoverable Begin,
Committed, and Report errors, while reserving fatal_error and terminal state for
errors that actually stop the worker.
- Around line 321-334: Replace the std::hint::spin_loop() pending path in the
proposal polling loop with a short sleep, capping the sleep duration by the time
remaining until deadline. Preserve immediate polling for non-PENDING statuses
and ensure the worker yields CPU while waiting so lifecycle commands can be
processed.
In `@crates/skippy-server/src/frontend/generation_flow.rs`:
- Around line 654-675: Move the GenerationStart emission block from before
request.lane_pool.checkout to immediately after the checkout succeeds,
preserving its existing configuration and token data. Keep checkout failure
propagation unchanged so no begin is emitted unless a lane is successfully
acquired.
In `@crates/skippy-server/src/frontend/generation_receipt.rs`:
- Around line 350-372: Update GenerationReceipt::record_token so budget and
non-monotonic timing violations are recorded rather than returned as errors.
Count rejected tokens, disable further receipt recording after the first
rejection, and allow both emit_token call sites to continue generation without
propagating bookkeeping failures; expose the count through delivery_failures or
an equivalent dedicated counter.
- Around line 173-195: Update the documentation for GenerationReceiptSink and
GenerationReceiptConfig::new to state that the queued adapter guarantees
ordering and at-most-once delivery, but not completeness or durable delivery
because pending observations may be lost during shutdown. Do not change the
implementation or add a JoinHandle unless implementing queue draining is
required; ensure accounting consumers are directed not to assume every
observation will be delivered.
- Around line 282-287: Remove the per-observation eprintln! from enqueue when
try_submit fails, while preserving the Relaxed submission_failures increment.
Use the existing delivery_failures accessor or telemetry path to expose the
accumulated count instead, and apply the same change to the analogous worker
submission path.
---
Outside diff comments:
In `@crates/mesh-llm-host-runtime/src/runtime/local_split/loading.rs`:
- Around line 342-352: Update stage_source_prepare_timeout to calculate the
transfer budget from the byte sizes of the artifacts selected by
required_stage_package_artifacts for the stage range, including optional
manifest, embeddings, outputs, and projectors when selected, rather than
distributing source_model_bytes uniformly across layers. Use the summed
selected-artifact bytes with the existing transfer-rate, allowance, and
minimum-timeout logic, and adjust callers to provide or derive that selection.
---
Nitpick comments:
In `@crates/mesh-llm-cli/src/parser/commands.rs`:
- Around line 501-503: Update the native_serving_plugin_deadline_ms argument
definition in the CLI command parser to use a value_parser that accepts only
positive u64 values, rejecting zero during argument parsing while preserving the
existing optional and dependency behavior.
In `@crates/mesh-native-serving-plugin-api/src/lib.rs`:
- Around line 152-204: Add an explicit ABI extension strategy for the lifecycle
and proposal event structs shown here: preferably add a leading struct_size
field to GenerationStart, GenerationCommit, GenerationAbort, GenerationFinish,
and ProposalQuery, matching the existing size-validation approach used by
ActivationContext and NativeServingPluginV1. Ensure the host can validate
truncated/older layouts so additive fields remain compatible; otherwise document
and implement a new ABI table for every layout change.
In `@crates/mesh-native-serving-plugin-host/src/lib.rs`:
- Around line 701-712: Update read_utf8 so it no longer returns an unbounded
borrowed &str from the raw ByteSlice; return an owned String instead, preserving
the existing null-pointer validation and UTF-8 error context, and adjust its
sole caller to use the owned value while retaining the current behavior.
In `@crates/openai-frontend/src/completions.rs`:
- Around line 47-74: Centralize the duplicated agent-session metadata logic by
adding or reusing one shared helper in common.rs. In
crates/openai-frontend/src/completions.rs lines 47-74 and
crates/openai-frontend/src/chat.rs lines 51-78, replace the local
set_agent_session, agent_session, and agent_session_source implementations with
the shared helper, and remove each file’s local agent-session key constants at
lines 14-15; preserve the strip-then-set behavior.
In `@crates/openai-frontend/src/responses.rs`:
- Around line 552-586: Rename the shared helper responses_conversation_cache_key
to responses_conversation_id and update both its prompt-cache and agent-session
call sites, including the usage in the normalization flow around
translate_openai_responses_input. Revise the AgentSessionSource documentation in
common.rs to reflect that the conversation ID is used as agent-session identity
and may also derive the prompt-cache key, without changing the existing capture
ordering.
In `@crates/skippy-server/src/frontend/generation_flow.rs`:
- Around line 636-640: Extract the split multimodal generation responsibility
centered on generate_split_multimodal_text from
crates/skippy-server/src/frontend/generation_flow.rs:636-640 into a semantically
named module such as generation_flow/split_multimodal.rs, moving its related
tests and preserving behavior; separately extract try_execute_linear_proposal
and its helpers from
crates/skippy-server/src/frontend/local_generation/token_generation.rs:84-101
into a module such as local_generation/linear_decode.rs, moving the associated
tests. Ensure both original files are reduced appropriately and each new module
remains under 1,000 lines.
In `@crates/skippy-server/src/frontend/generation_receipt.rs`:
- Around line 876-901: Replace
receipt_lifecycle_begins_before_generation_and_closes_before_cleanup’s
include_str!/source-offset checks with a behavioral test using a recording
GenerationLifecycleIngress. Drive one local generation and record lifecycle
events, then assert the sequence contains Started, Committed, and a terminal
Completed or Aborted event, with cleanup observed last; avoid asserting source
text or exact local expressions.
In `@crates/skippy-server/src/frontend/local_generation/tests.rs`:
- Around line 208-234: Extend the failing-sink test around `sink.fail` and
`failing_ids` to assert that the configured
`GenerationReceiptConfig::delivery_failures` counter increments after the failed
receipt is processed. Keep the existing session cleanup assertion, and ensure
the check occurs after waiting for asynchronous receipt handling.
In `@crates/skippy-server/src/frontend/local_generation/token_generation.rs`:
- Around line 94-101: Change prompt token storage in GenerationStart and
GenerationReceipt to Arc<[i32]> so the existing prompt allocation can be shared
instead of copied by both receipt paths. Update the construction and propagation
in the surrounding generation flow, including build_generation_receipt, while
preserving the exact token evidence; verify the plugin ABI conversion in lib.rs
remains unchanged.
In `@crates/skippy-server/src/lib.rs`:
- Around line 32-37: Remove LinearProposalIngressConfig from the crate-root
re-export list in lib.rs, matching GenerationReceiptConfig’s module-owned access
through frontend. Keep the type available via its owning module so
ModelServingHooks::new callers use consistent import paths.
🪄 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: 72d78506-c801-4aac-95c2-fad8d3ae0a67
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
Cargo.tomlcrates/mesh-llm-cli/src/parser/commands.rscrates/mesh-llm-host-runtime/Cargo.tomlcrates/mesh-llm-host-runtime/src/inference/skippy/mod.rscrates/mesh-llm-host-runtime/src/lib.rscrates/mesh-llm-host-runtime/src/runtime/local.rscrates/mesh-llm-host-runtime/src/runtime/local_model_only.rscrates/mesh-llm-host-runtime/src/runtime/local_split.rscrates/mesh-llm-host-runtime/src/runtime/local_split/coordinator.rscrates/mesh-llm-host-runtime/src/runtime/local_split/loading.rscrates/mesh-llm-host-runtime/src/runtime/local_split/test_support.rscrates/mesh-llm-host-runtime/src/runtime/local_split/tests.rscrates/mesh-llm-host-runtime/src/runtime/options.rscrates/mesh-llm/src/lib.rscrates/mesh-native-serving-plugin-api/Cargo.tomlcrates/mesh-native-serving-plugin-api/README.mdcrates/mesh-native-serving-plugin-api/src/lib.rscrates/mesh-native-serving-plugin-host/Cargo.tomlcrates/mesh-native-serving-plugin-host/README.mdcrates/mesh-native-serving-plugin-host/src/lib.rscrates/openai-frontend/src/chat.rscrates/openai-frontend/src/common.rscrates/openai-frontend/src/completions.rscrates/openai-frontend/src/lib.rscrates/openai-frontend/src/responses.rscrates/openai-frontend/src/router.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/backend.rscrates/skippy-server/src/frontend/generation/cache_hints.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/generation_receipt.rscrates/skippy-server/src/frontend/linear_proposal.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/local_generation/tests.rscrates/skippy-server/src/frontend/local_generation/token_generation.rscrates/skippy-server/src/frontend/tests/generation.rscrates/skippy-server/src/lib.rscrates/skippy-server/src/serving_hooks.rsscripts/affected-crates.shscripts/plan-clippy-batches.shscripts/publish-crates.shwebsite/src/docs/pages/CLI.md
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/mesh-llm-host-runtime/src/runtime/local.rs
- crates/mesh-llm-host-runtime/src/inference/skippy/mod.rs
- crates/skippy-server/src/frontend/linear_proposal.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/skippy-server/src/frontend/generation_flow.rs (1)
250-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the receipt lifecycle into one shared helper.
generate_multimodal_textandgenerate_split_multimodal_textnow repeat the same five steps: tokenize the prompt intoArc<[i32]>, emitbegin, create the observation throughconfig.observation, record and commit each token, then finalize withgeneration_succeeded. Lines 250-260 mirror lines 464-474, lines 299-309 mirror lines 705-715, and lines 416-432 mirror lines 878-891.Both functions also remain very long after this change. Extract the lifecycle into a helper that owns the prompt tokens, the observation, and the terminal event. That removes the duplication and reduces the chance that one path diverges from the other on a later edit.
Also applies to: 299-309, 416-432
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/generation_flow.rs` around lines 250 - 260, Extract the duplicated generation-receipt lifecycle from generate_multimodal_text and generate_split_multimodal_text into one shared helper. Have the helper tokenize and own the Arc<[i32]> prompt tokens, call config.begin and config.observation, record and commit each token, and emit generation_succeeded; replace the repeated blocks at the referenced begin, observation/record/commit, and finalization sites while preserving each function’s existing request and session identifiers.crates/skippy-server/src/frontend/local_generation.rs (1)
66-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClose the terminal-event hole when the observation is absent.
Line 78 returns
Ok(())whenconfigexists butobservationisNone. The callers emitbeginwheneverself.generation_receiptisSome. In that combination the sink receivesbeginwith norecordand noabort.The provided callers always create an observation when the config exists, so this branch is currently unreachable. The type does not enforce that. Emit
abortin theNonebranch so the contract holds for any future caller.♻️ Proposed change: abort instead of returning silently
match finalization.observation { Some(observation) => { self.deliver_local_generation_receipt(LocalGenerationReceiptDelivery { config, session_label: finalization.session_label, request_id: finalization.request_id, session_id: finalization.session_id, agent_session_id: finalization.agent_session_id, prompt_token_ids: finalization.prompt_token_ids, observation, }) } - None => Ok(()), + None => { + config.abort(crate::frontend::GenerationAbort { + request_id: finalization.request_id, + session_id: finalization.session_id, + }); + Ok(()) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/skippy-server/src/frontend/local_generation.rs` around lines 66 - 79, Update the None branch in the finalization observation match within deliver_local_generation_receipt to emit an abort event when a generation receipt config exists but no observation is present, instead of returning Ok(()) silently. Preserve the existing receipt delivery for Some(observation) and ensure the terminal event contract is completed for this case.
🤖 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-server/src/frontend/generation_flow.rs`:
- Around line 250-260: Extract the duplicated generation-receipt lifecycle from
generate_multimodal_text and generate_split_multimodal_text into one shared
helper. Have the helper tokenize and own the Arc<[i32]> prompt tokens, call
config.begin and config.observation, record and commit each token, and emit
generation_succeeded; replace the repeated blocks at the referenced begin,
observation/record/commit, and finalization sites while preserving each
function’s existing request and session identifiers.
In `@crates/skippy-server/src/frontend/local_generation.rs`:
- Around line 66-79: Update the None branch in the finalization observation
match within deliver_local_generation_receipt to emit an abort event when a
generation receipt config exists but no observation is present, instead of
returning Ok(()) silently. Preserve the existing receipt delivery for
Some(observation) and ensure the terminal event contract is completed for this
case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 960faa6c-5cbb-48c3-a88a-4797a4ace443
📒 Files selected for processing (19)
crates/mesh-llm-cli/src/parser/commands.rscrates/mesh-llm-host-runtime/src/models/artifact_transfer.rscrates/mesh-llm-host-runtime/src/runtime/local_split/loading.rscrates/mesh-llm-host-runtime/src/runtime/local_split/tests.rscrates/mesh-native-serving-plugin-api/README.mdcrates/mesh-native-serving-plugin-api/src/lib.rscrates/mesh-native-serving-plugin-host/src/lib.rscrates/openai-frontend/src/chat.rscrates/openai-frontend/src/common.rscrates/openai-frontend/src/completions.rscrates/openai-frontend/src/responses.rscrates/skippy-server/src/frontend/generation_flow.rscrates/skippy-server/src/frontend/generation_flow/text_generation.rscrates/skippy-server/src/frontend/generation_receipt.rscrates/skippy-server/src/frontend/local_generation.rscrates/skippy-server/src/frontend/local_generation/decode_step.rscrates/skippy-server/src/frontend/local_generation/linear_decode.rscrates/skippy-server/src/frontend/local_generation/token_generation.rscrates/skippy-server/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/skippy-server/src/lib.rs
- crates/openai-frontend/src/chat.rs
- crates/openai-frontend/src/responses.rs
- crates/openai-frontend/src/completions.rs
- crates/mesh-llm-cli/src/parser/commands.rs
- crates/mesh-native-serving-plugin-host/src/lib.rs
- crates/mesh-native-serving-plugin-api/src/lib.rs
- crates/skippy-server/src/frontend/generation_receipt.rs
3 participants
Use case
A host application integrating with Skippy may need accurate progress metrics, request accounting, speculative-resolution telemetry, or cleanup on cancellation. A benchmark runner can use the same events to separate committed generation from attempted work without parsing logs.
Change
Optional typed hooks now report authoritative generation transitions: start, committed token progress, proposal resolution, completion, and abort. Events carry correlation data and are emitted where Skippy commits each transition.
Skippy continues to own model execution, tokenization, verification, scheduling, and cleanup. The hooks are absent by default and cannot replace or control those responsibilities.
This PR deliberately does not expose an alternate Mesh launcher or an injected local-model entrypoint. The normal Mesh executable and serving startup path remain unchanged; a separate focused draft will propose any native plugin loading boundary.
Emitting typed events at the authoritative transition points gives integrations a stable view of what actually happened without duplicating generation logic.
Validation
just with-lld cargo check -p mesh-llm -p mesh-llm-host-runtime -p skippy-serverjust with-lld cargo test -p skippy-server(352 passed)git diff --check.Summary by CodeRabbit
New Features
Bug Fixes
Documentation