feat(mtmd,tests): multimodal segment rail + integration coverage by lloyal-research · Pull Request #36 · lloyal-ai/liblloyal

lloyal-research · 2026-08-30T06:21:15Z

Builds on 42e5951 (embedding-rail primitives), moving the multimodal walk off the binding and into the kernel — then covering it with real evals.

Why

The mtmd pipeline lived in lloyal.node's N-API worker: ~120 lines of chunk-walking, position packing and cell bookkeeping, with BranchState::position crossing the boundary. Any other SessionContext implementation (Nitro/JSI, a platform encoder) would have had to reimplement all of it.

What

BranchStore::decode_segments(handle, SegmentSource&) — drives a heterogeneous prefill: TEXT segments through the token rail, EMBD through decode_embd, logits requested on the final segment. The branch position never leaves the class.
MtmdSource, in the opt-in <lloyal/mtmd.hpp> — adapts llama.cpp's mtmd to decode::SegmentSource. The one place the two vocabularies meet; branch.hpp and decode.hpp never see an mtmd_* type. liblloyal links nothing on a consumer's behalf: a target that includes this header links mtmd itself, and a consumer that doesn't include it carries no multimodal dependency.
Scratch gains an embedding twin (as_embd_batch) and section-major position sizing. decode::embd now fills Scratch per view rather than repacking into a side buffer.
Cell/position accounting. An M-RoPE image occupies n_rows KV cells but advances position by only n_pos = max(nx, ny). The difference is tracked as slack (img_slack_own / img_slack_total) so the pressure gauge stays exact across fork, release and retainOnly.

Bug fixed

mtmd_tokenize's header documents 1 for a marker/image count mismatch, but that check throws from mtmd_tokenizer's constructor and mtmd_tokenize's catch-all reports 2 — indistinguishable from a real preprocessing failure. Callers passing mismatched counts were told "image preprocessing failed". MtmdSource now counts markers itself and reports both numbers.

Tests

Four integration cases against a real VL pair, in multimodal_integration_test.cpp:

position/cells decouple — probes mtmd_decode_use_mrope and asserts the right invariant per model, then that prune recovers the slack rather than the position delta.
retainOnly promotes inherited slack — the media analogue of the existing retainOnly resets fork_head case; without the promotion the final release under-subtracts and the gauge never returns to zero.
fan-out — one encode, 4 forks costing zero cells, four different questions bin-packed into one scatter, one decode_each per tick, then per-child release recovering only its own cells with the image prefix intact.
error paths — null context, undecodable bytes, and count mismatch in all three directions.

The rest of this suite asserts cells_used == <token count> throughout, because on the token rail cells and position are the same number. An image is the first thing that separates them — that gap is what these cases exist to hold.

Two tiers. Qwen3.5-4B (M-RoPE) asserts answer content — 4/4, 58 assertions. SmolVLM-256M runs mechanics only via LLOYAL_VL_STRICT=0 and covers the plain-position rail the Qwen tier never reaches — 4/4, 54 assertions. Full suite 167/167, 252,093 assertions.

Unit tier restored. decode::embd calls llama_set_causal_attn, which the stubs lacked — TestRunner had not built since 42e5951. Now 256/256, 1,276 assertions, with causal_attn_log making the non-causal bracket assertable.

CI

LLAMA_BUILD_TOOLS is off in build-llama.sh, so tools/mtmd declares no targets. A dedicated step enables it and builds only the mtmd target — no tool CLIs — into the same tree as libllama, since two trees would mean two backend registries in one process. build-llama.sh is untouched. The SmolVLM pair (274 MB) joins the existing cached fixtures step, and multimodal runs as its own step.

ctest TIMEOUT 180 → 1800: these are real inference runs, and 180s killed runs that pass when IntegrationRunner is invoked directly.

Follow-up (not in this PR)

The stub tier still has no decode::embd / decode_segments cases. The integration tier covers them; causal_attn_log now makes the non-causal bracket assertable there too.

…eeping Text and images are two input rails into the same KV cache; this adds the embedding rail beside the token rail. - decode::embd — inject pre-computed embedding rows into one sequence's KV via batch.embd (a llama_batch is token-XOR-embd, so rows are always their own dispatch). Section-major positions carry M-RoPE's 4-wide layout; internal sub-chunking by n_batch; llama_set_causal_attn bracket for non-causal projectors; want_last_logits for rows-terminal prefills. - BranchStore::prefill_embd — branch-level wrapper with the one multimodal bookkeeping difference: cells grow by n_tokens while position advances by n_pos (max(nx,ny) under M-RoPE). The gap is tracked as embedding-row slack (img_slack_own / img_slack_total, inherited on fork) so release() and retainOnly() recover exact cell counts. - decode_scatter — duplicate-handle assert: two items for one branch would read the same start_pos and collide; sequential calls required. - chat_in — content_parts guard on both empty-system strip sites (primary and sentinel-retry): a parts-based system message (text + media_marker) keeps its content string empty while carrying parts — that is a real system prompt, not a suppression request. - README — the embedding rail section + decode grid row.

42e5951 landed the embedding-rail primitives; this moves the multimodal walk off the binding and into the kernel, and covers it. - BranchStore::decode_segments(handle, SegmentSource&) drives a heterogeneous prefill: TEXT segments through the token rail, EMBD through decode_embd, logits on the final one. The branch position never leaves the class. - MtmdSource (opt-in <lloyal/mtmd.hpp>) adapts llama.cpp's mtmd to decode::SegmentSource, so every SessionContext implementation shares one walk instead of reimplementing it. liblloyal links nothing on its behalf — linking is the consumer's job. - Scratch grows an embedding twin (as_embd_batch) and section-major position sizing; decode::embd fills per view instead of repacking. - Marker/image count is now checked up front. mtmd documents rc==1 for a mismatch, but that check throws from mtmd_tokenizer's constructor and mtmd_tokenize's catch-all reports rc==2 — so callers were told "image preprocessing failed" for a plain count error. Tests: 4 integration cases against a real VL pair — the cells/position split an image is the first thing to cause, slack promotion through retainOnly, the fan-out (one encode, N forks at zero cells, N questions, one dispatch per tick), and the error paths. Two tiers: Qwen3.5-4B asserts answer content; SmolVLM-256M runs mechanics only and covers the plain-position rail. Also restores the unit tier: decode::embd calls llama_set_causal_attn, which the stubs lacked, so TestRunner had not built since 42e5951. CI builds only the mtmd target in llama.cpp's tree (tools are otherwise off) and runs the multimodal cases against the SmolVLM pair. ctest TIMEOUT 180 -> 1800: these are real inference runs, not stubs.

Copilot

Pull request overview

Moves multimodal (text + embedding-row) prefill orchestration into the C++ kernel so all bindings can share the same segment-walk logic, and adds end-to-end integration coverage against real VL model fixtures.

Changes:

Add an embedding-row decode primitive (decode::embd) plus BranchStore wrappers for embedding-prefill and heterogeneous segment-prefill (decode_segments) with KV cell/position slack accounting.
Introduce an opt-in mtmd adapter header (<lloyal/mtmd.hpp>) implementing decode::SegmentSource without leaking mtmd types into core headers.
Add multimodal integration tests + CI plumbing (mtmd build, additional fixtures, dedicated multimodal test step, longer ctest timeout).

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/stubs/llama_stubs.h	Adds stub declaration/state for `llama_set_causal_attn` to support embedding-rail tests.
tests/stubs/llama_stubs.cpp	Implements `llama_set_causal_attn` stub with transition logging.
tests/integration/multimodal_integration_test.cpp	New integration suite validating embedding rail mechanics, slack accounting, fan-out behavior, and error paths.
tests/CMakeLists.txt	Wires multimodal integration test in, adds mtmd include/lib discovery, and increases ctest timeout.
README.md	Documents the embedding rail and updates the decode grid/table.
include/lloyal/mtmd.hpp	New opt-in mtmd-backed `decode::SegmentSource` adapter for multimodal segment production.
include/lloyal/decode.hpp	Adds `Scratch` embedding-batch support plus `decode::embd` and segment-source interfaces.
include/lloyal/chat_in.hpp	Prevents stripping “empty” system messages when they actually contain content parts (e.g., media markers).
include/lloyal/branch.hpp	Adds image slack bookkeeping, duplicate-handle guard in `decode_scatter`, embedding decode wrapper, and `decode_segments`.
.github/workflows/tests.yml	Builds mtmd in CI, downloads VL fixtures, splits multimodal tests into a dedicated step.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

lloyal-research · 2026-08-30T07:58:58Z

+#include <cstdint>
+#include <cstdio>
+#include <cstdlib>
+#include <doctest/doctest.h>


Fixed in d84f249 — <cctype> added explicitly rather than relying on it arriving transitively.

lloyal-research · 2026-08-30T07:58:59Z

+                         const std::vector<std::string>& needles) {
+  std::string lower;
+  lower.reserve(haystack.size());
+  for (char c : haystack) lower += static_cast<char>(std::tolower(c));


Fixed in d84f249. Iterating as unsigned char now. Reachable rather than theoretical: the haystack is model output, so any UTF-8 continuation byte (>= 0x80) hits it on a platform where char is signed.

lloyal-research · 2026-08-30T07:59:02Z

+```cpp
+// A multimodal prefill interleaves sequential calls per branch:
+store.decode_scatter({{h, text_before}});          // token rail
+store.prefill_embd(h, rows, n_tokens, n_embd_inp,  // embedding rail


Fixed in d84f249, same rename.

lloyal-research · 2026-08-30T07:59:04Z

+   * Logits are captured on the FINAL segment only: every `llama_decode`
+   * resets the output buffer, so an interior segment's logits are dead. A
+   * trailing TEXT segment gets them via `decode_scatter`'s per-item capture;
+   * a trailing EMBD segment via `decode_embd`'s `want_logits`.


Fixed in d84f249. You were right that the doc overstated it: decode_scatter sets output_logits per dispatch, so interior TEXT segments do capture — their snapshots are just overwritten. Reworded to say only the final segment's survive, and to name why (each llama_decode resets the output buffer).

lloyal-research

The segment-source boundary is a strong improvement: production stays adapter-specific while placement, branch position, KV accounting, and topology remain in liblloyal. The slack ownership model also looks internally coherent across fork, release, and retainOnly.

I would treat the P1 below as blocking merge. The other inline findings cover gaps at the new public/runtime boundary:

non-causal vision blocks are currently allowed to split despite being semantically indivisible;
required CI never produces nonzero embedding slack, so it does not exercise the accounting this PR introduces;
SegmentSource geometry reaches allocation/callback code before it is validated;
non-causal mode restoration is not exception-safe.

Please also resolve the existing inline threads covering the nonexistent prefill_embd documentation, std::tolower correctness, and the final-segment logits contract. The last one has a runtime cost as well as a documentation mismatch: each interior TEXT segment currently computes the LM head and copies a full vocabulary snapshot.

…idation, real slack coverage Addresses review on #36. P1 — a non-causal block was auto-chunked (decode::embd). The chunk loop split by n_batch regardless of item.non_causal, and llama_set_causal_attn(false) wrapped the LOOP. Each llama_decode is a separate forward pass, so rows in an earlier chunk cannot attend to later ones: the block silently stops being bidirectional and the vision state is wrong with no error. Now requires n_rows <= min(n_batch, n_ubatch) for non_causal and refuses otherwise. Latent in practice — use_non_causal is GEMMA3/4V/4UV only, and the shipped catalog is Qwen (qwen3vl_merger) and SmolVLM — but the code accepted a configuration it could not honour. P1 — causal mode is now restored by an RAII guard. scratch.resize() can throw between set(false) and the restore, which only ran on rc != 0 or normal completion. Causal mode is context-wide, so a caller that caught and continued would decode all SUBSEQUENT TEXT non-causally. P1 — decode_segments validates segment geometry before it is used. SegmentSource is a public extension point, so its geometry is untrusted: n_pos_per_embd of -1 wrapped the size_t multiply into an enormous allocation, and 0 handed positions() a zero-length buffer to write into. decode_embd's checks ran too late. Validates n_pos_per_embd in {1,4}, n_embd_inp > 0, 0 < n_pos <= n_rows before assign() and the callback. P2 — the required CI tier could not exercise embedding-row slack. SmolVLM is plain-position, so n_pos == n_rows and every img_slack_* field stays zero: the release/retainOnly tests passed with the accounting deleted. Adds four stub-tier cases that force n_rows > n_pos deterministically, plus llama_n_ubatch to the stubs and settable batch geometry. Verified by mutation rather than assertion count — with the tests in place: release ignoring img_slack_own -> 1 failed retainOnly not promoting the slack -> 1 failed Both keep a second branch alive, because release() zeroes cells_used_ outright once the last branch goes and would otherwise mask the arithmetic entirely. Also: <cctype> for std::tolower and unsigned char to avoid UB on UTF-8 continuation bytes; README named the pre-rename prefill_embd; the decode_segments doc claimed logits are captured only on the final segment when decode_scatter captures per dispatch and interior captures are overwritten.

It is the first function-local struct in these headers, so there was no precedent to follow. Namespace-scope types here carry /// blocks and default member initializers; the guard now matches, and says why restoring at the return points is insufficient (causal mode is context-wide, and resize() can throw).

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

lloyal-research · 2026-08-30T07:58:40Z

+    if (want_logits) {
+      const float* raw_logits = logits::get(state->ctx, -1);  // throws if absent
+      if (state->n_vocab <= 0) {
+        throw std::runtime_error("BranchStore::decode_embd - invalid vocab size");
+      }
+      assert(state->logits_snapshot.size() >= static_cast<size_t>(state->n_vocab));
+      std::memcpy(state->logits_snapshot.data(), raw_logits,
+                  state->n_vocab * sizeof(float));
+      state->has_logits = true;
+    }


Fixed in b9be568. Confirmed: decode_each (994) and decode_scatter (1081, 1112) capture on every dispatch, so decode_embd was the only rail that could advance the position while leaving has_logits/logits_snapshot describing an earlier one. It now clears has_logits when no logits were requested. decode_segments was already safe in practice — its final segment always refreshes — but decode_embd is public and callable directly. Stub case added; mutation-verified (dropping the clear → 1 failed).

An embedding decode with want_logits=false advanced the position and the KV while leaving has_logits and logits_snapshot describing an EARLIER position. sample() would then read logits that no longer belong to where the branch is — wrong, and silently so. decode_each and decode_scatter capture on every dispatch, so decode_embd was the one rail of the three that could strand them. It now clears has_logits when no logits were requested. decode_segments was already safe in practice — its final segment always refreshes, whether that is a TEXT segment through decode_scatter or a trailing EMBD with want_logits — but decode_embd is public and callable directly. Stub case added; verified by mutation (dropping the clear -> 1 failed).

lloyal-research

Focused re-review of the latest head, plus a holistic pass across the surrounding decode, KV, logits, sampler, grammar, tokenizer, chat-input, and build seams.

The updates resolve the earlier review well: the causal-mode RAII guard is exception-safe, non-causal geometry is validated before allocation and dispatch, slack ownership and fork accounting are covered by useful mutation tests, and the latest has_logits reset closes the stale-logits path. The current CI matrix is green.

I found three remaining contract issues:

P1 / blocking: embedding row width is trusted even though the native batch carries no width metadata. This can become a wrong-stride or out-of-bounds native read.
P2: terminality is computed before empty text segments are skipped, and interior text still requests/copies logits.
P2: unsupported audio is rejected lazily, after earlier segments may already have advanced the branch.

Architecturally, the overall seam is sound. SegmentSource keeps codecs and mtmd types out of the kernel while still allowing a native multimodal model to receive actual embedding rows; sampler and grammar state correctly advance only when tokens are sampled; branch forking clones KV/sampler/grammar/logits and the new slack accounting consistently; tokenizer flags match the existing special-token contract; the chat content_parts guard is appropriate; and opt-in mtmd linkage against the same llama build tree is the right build boundary.

Non-blocking cleanup for this PR or a follow-up:

Rename img_slack_* to modality-neutral cell_slack_* or position_slack_*.
Update Branch::position() documentation: under M-RoPE it is a logical position, not necessarily a token count.
Tighten MtmdSource lifetime docs: image bytes are consumed during construction rather than retained; the mtmd context and separator are the borrowed objects.
Add the exact chat regression case: a system message represented with content_parts, including media.
Refresh the PR body: its stub-test counts and “final-only logits” statement are now stale.

Because the connected account owns the PR, GitHub records this as a Comment review rather than Request changes; I consider the P1 item merge-blocking.

…d up front Second review round on #36. P1 — decode::embd validated n_embd_inp only as positive. llama_batch carries no row-width metadata: llama_decode consumes rows at the MODEL's input width while the chunk loop strides by the caller's, so a wrong-but-positive width starts later chunks mid-row and reads past the caller's allocation. Now compared against llama_model_n_embd_inp(llama_get_model(ctx)). P2 — terminality was positional but computed before empty segments were skipped. A source yielding [EMBD, empty TEXT] decoded the image with want_logits=false, skipped the tail, and returned a branch that could not sample. decode_segments now rejects empty segments outright, which makes is_last correct by construction. Verified mtmd never produces one, including for a bare-marker prompt — the shape most likely to yield an empty leading chunk (it yields two non-empty TEXT segments). P2 — MtmdSource accepted audio into the chunk list and deferred the error to at(), by which point decode_segments may have committed preceding chunks and left the branch partially advanced. It now scans the tokenized chunks in the constructor, before any dispatch can happen. decode_segments' contract now states that it is NOT atomic: once the first segment dispatches, a later throw leaves the branch poisoned, and the caller must prune rather than continue from it. Stubs gain llama_model_n_embd_inp (0 = no opinion, so existing cases are unaffected). New cases cover the width mismatch and the bare-marker shape. NOT addressed here, deliberately: carrying want_logits through the text rail so interior TEXT segments skip the LM-head pass. That changes decode_scatter's contract, which the whole SDK prefill path uses — worth doing, but on its own.

lloyal-research

Re-reviewed the current head (12e5bb8) against the three findings and the surrounding decode/branch/mtmd contracts.

All three correctness issues are resolved:

decode::embd now validates the caller's row width against llama_model_n_embd_inp(llama_get_model(ctx)) before touching the source buffer. That is the exact width llama.cpp uses when it initialises an embedding batch.
decode_segments rejects empty segments, making positional terminality sound by construction without violating SegmentSource's in-order/reusable-buffer contract. The bare-marker integration case validates the real mtmd source's most suspicious shape.
MtmdSource rejects audio during construction, before streaming decode can mutate the branch, and decode_segments now documents the correct poisoned-branch recovery contract for later source/decode failures.

The earlier non-causal, RAII, geometry, slack-accounting, and stale-logits fixes remain correct in combination with these changes. I also rechecked the sampler/grammar/logits and KV ownership seams: image rows enter as native model-input embeddings, do not spuriously advance grammar or repetition state, terminal logits still gate sampling, and fork/release/retain accounting remains coherent.

I agree with deferring the interior-TEXT want_logits optimisation: the wasted LM-head pass and vocabulary copy are real, but changing decode_scatter's shared contract deserves a focused follow-up rather than widening this fix.

No new findings. Unit, sanitizer, and style jobs are green; the integration job is still running at the time of this review. From the code-review side, this is ready once the remaining CI job passes.

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

include/lloyal/mtmd.hpp:71

ctx_ is also borrowed and is used by later at()/positions() calls, whereas the image byte vectors are consumed during construction and are not retained. The current lifetime contract omits the object that must outlive MtmdSource and unnecessarily requires the byte buffers to remain alive.

 * **Lifetime.** Owns its bitmaps and chunk list. `sep` and the byte vectors
 * are borrowed — the caller must outlive this object. Satisfies
 * SegmentSource's in-order contract: an Embd segment's `rows` point into
 * mtmd's context-owned encode buffer, which the *next* `at()` overwrites.

tests/CMakeLists.txt:315

This unconditional requirement breaks the repository's documented local integration workflow: .github/scripts/build-llama.sh:57 builds with LLAMA_BUILD_TOOLS=OFF, and both .github/scripts/setup-llama-cpp.sh:9-13 and README.md:354-357 then configure IntegrationRunner without building mtmd. Update that workflow to build mtmd in the same tree, or gate the multimodal source/link requirement behind a dedicated test option.

  if(NOT MTMD_LIB)
    message(FATAL_ERROR
      "libmtmd not found — the multimodal integration tests need it.\n"
      "Build it in the same tree as libllama:\n"
      "  cmake ${LLAMA_CPP_BUILD_DIR} -DLLAMA_BUILD_TOOLS=ON -DMTMD_VIDEO=OFF\n"
      "  cmake --build ${LLAMA_CPP_BUILD_DIR} --target mtmd\n"

include/lloyal/chat_in.hpp:188

No test exercises the behavior these guards add: the existing content_parts array integration case uses a user message, while this branch is specific to a system message whose string content is empty. Add system-message content-parts cases for the normal and synthetic-user retry paths so either guard cannot regress silently.

      if (!messages.empty() && messages[0].role == "system" &&
          messages[0].content.empty() && messages[0].content_parts.empty()) {

lloyal-research · 2026-08-30T09:07:00Z

+      // chunk and collide on start_pos (overlapping KV positions). Callers
+      // with multiple runs for one branch must issue sequential calls.
+      for (int32_t j = 0; j < i; ++j) {
+        if (items[j].handle == items[i].handle) {


Fixed in 273e808 — empty spans are now excluded from the duplicate check, since bin_pack skips them and a span that never decodes cannot collide on start_pos. Only two spans that both decode can.

One correction on the framing: the contract is thinner than "documented and tested". The test at branch_integration_test.cpp:2440 passes an empty items vector, not an item with an empty token span, and the only documentation is the internal comment "bin_pack skips empties internally". So this was a spurious throw rather than a broken guarantee. Worth fixing either way, and it now has a stub case both directions (empty + real passes; real + real still throws), mutation-verified.

lloyal-research · 2026-08-30T09:07:03Z

+    if (decode::embd(state->ctx, item, state->n_batch, scratch_) != 0) {
+      throw std::runtime_error("BranchStore::decode_embd - llama_decode failed");


Fixed in 273e808, and made atomic rather than documented. Confirmed exactly as described: the throw at :1182 precedes the position/cells_used_/slack updates at :1185-1189, while decode::embd has already committed earlier chunks. Worth adding that this is the MAIN path rather than an edge — the integration fixture is ~550 rows against a default n_batch of 512, so multi-chunk is normal.

Took the rollback option over poisoning: decode_embd records the position it started from and, on failure, calls kv::remove_range(seq, start, -1) to strip everything the call wrote, so the branch is left exactly as it was found and a retry is safe. Stubs now record seq_rm calls (mirroring the existing seq_cp tracking) and a case asserts the stripped range is [start, -1) on the right seq_id, with position, cells and slack all unmoved. Mutation-verified: dropping the rollback fails it.

Walls of text replaced with 11 mermaid diagrams (GitHub renders these natively) covering the abstractions that were previously prose-only: tree batching's N-to-1 dispatch, decode_scatter's bin-packing, BranchStore's slot/lease/registry layout, the branch lifecycle, tenancy's abundant-vs-scarce split, topology, the two-rail multimodal prefill, and the cells-vs-position split. Adds the multimodal architecture section: the SegmentSource portability seam, why an image becomes forkable live state rather than a resent attachment, and the decode invariants the embedding rail enforces. Three code bugs fixed along the way — the samples did not compile: - store.decode_each/decode_scatter take DecodeEachItem/DecodeScatterItem (branch.hpp), not decode::EachItem/ScatterItem (decode.hpp). Those are a lower layer taking raw seq_ids and are not interchangeable. Three samples used the wrong one; a note now states the distinction. - reseed_chain(handle, store, seed) does not exist. The only reseed_chain is sampler::reseed_chain(llama_sampler*, uint32_t). Forks diverge via set_sampler_params with a changed seed, which the memoization treats as a rebuild. Two samples called the phantom overload. - The CMake target is liblloyal::liblloyal, not `lloyal`. Test counts refreshed (256/128 -> 262/167) and the integration invocation corrected to build_integration with the multimodal env vars. Multimodal claims verified against source rather than restated: the mtmd projector runs lazily in at(), not in the constructor (which is what allows the encoder to reuse one output buffer); n_pos comes from mtmd_input_chunk_get_n_pos, not a formula we compute; and fork clones the logits snapshot by default rather than leaving the child empty.

The stateDiagram-v2 version had back-edges (Forked->Live, Promoted->Live) which made mermaid stack edge labels on top of each other on GitHub: 'retainOnly(winner)' overlapped 'winner becomes trunk', and 'independent decode' overlapped 'fork()'. Rendered and checked in the browser. Redrawn as an LR cycle with the text inside nodes instead of on edges, which is what auto-layout can place reliably.

…two API instructions Second review round on the README. Each verified against source before changing. - Slot/lease was wrong, and had been since before this rewrite: allocate() acquires the LEASE first (branch.hpp:461), fails when none is free (:462) and rolls it back if the slot allocation fails (:465). Slot and lease are taken atomically, so every live branch is KV-resident and simultaneous branches are bounded by n_seq_max — not by the 65,535 slot table. The old "slots are how many can exist, leases how many can decode" framing implied dormant non-resident branches, which cannot occur. The KV-tenancy diagram taught the same wrong model as movement from slot to lease; it is gone, and the ownership diagram now shows both being acquired together. - The multimodal diagram had MtmdSource FEEDING SegmentSource. It implements it. Prompt and media enter MtmdSource, which exposes the contract to BranchStore — now drawn with implements edges, and with a custom source beside it to make the seam's purpose visible. - decode_each() is three plain loops: no n_batch check, no bin_pack, no chunking. The one-dispatch claim is now conditioned on one row per active branch fitting the configured batch, and says sizing is the caller's. - Dropped "PCIe round-trips" — meaningless on the unified memory of Apple Silicon, which this library prominently supports. "Dispatch and synchronization overhead" holds everywhere. - git submodule add -b v0.1.0 referenced a tag that does not exist; tags run v1.0.0-alpha to v1.5.4. Tag pin dropped. - clone_logits was documented as if reachable from Branch::fork(), which takes no argument. It is a ForkOpts field on branch::fork(source, store, opts), which the note now shows. Diagrams cut 11 -> 5: continuous tree batching, BranchStore ownership, the multimodal segment architecture, the cells/position split, and the tree-search cycle. The packing, ordered-prefill, lifecycle and topology diagrams read better as the prose and tables already beside them.

…handle Third review round on #36. decode::embd chunks by n_batch, so a failure on a later chunk left EARLIER chunks committed to the sequence while position, cells_used_ and slack were never updated — the throw happens before them. A caller that caught it and retried would decode over rows already resident, silently overlapping positions. This is the main path, not an edge: our fixture is ~550 rows against a default n_batch of 512. decode_embd now records the position it started from and, on failure, calls kv::remove_range(seq, start, -1) to strip everything the call wrote. The branch is left exactly as it was found, so the operation is atomic rather than merely documented as poisoning. The duplicate-handle guard also rejected a pair where one span is empty. bin_pack skips empty spans, so they occupy no cells and cannot collide on start_pos — only two spans that both decode can. Empty spans are now excluded from the check. Stubs record seq_rm calls, mirroring the existing seq_cp tracking, so a test can assert the rollback stripped exactly the right range. Verified by mutation: dropping the empty-span exclusion -> 1 failed dropping the rollback -> 1 failed Unit 264/264; multimodal, decode error-path and retainOnly integration cases 9/9 (126 assertions).

Auditing the README's decode invariants against the suite found four with no test — including the geometry validation, which is a P1 memory-safety fix that shipped untested. Added, all mutation-verified: - decode_segments validates n_pos_per_embd, n_embd_inp and n_pos BEFORE the source callback. Asserts positions() was never reached, which is the actual property: -1 wraps the size_t multiply and 0 hands the callback a zero-length buffer, so validating after the call is worthless. - decode_segments rejects an empty segment (terminality is positional). - causal mode is restored when the decode FAILS, not just on success. The existing case only covered the happy path, which is the one that never mattered — the guard exists for the throw. - audio is rejected before the branch is touched. The audio case corrected an assumption of mine. With a vision-only projector mtmd_support_audio() is false, so mtmd refuses to build an audio bitmap and rejection happens at DECODE — earlier than the constructor's chunk scan I added last round. That scan is a second line of defence, reachable only with an audio-capable mmproj. The test asserts the path that actually fires and says why. Two CI sanitizer gaps, found while answering whether sanitizers should have caught the recent findings: - The sanitizer job ran on the default GCC, which takes the address+undefined branch. -fsanitize=integer and -fsanitize=float-divide-by-zero are Clang only (integer is a Clang group alias GCC rejects), so the unsigned-overflow and implicit-truncation checks the run step advertises were never enabled — including the ones that would flag the size_t wraparound above. Now runs on clang. - LLOYAL_ENABLE_UBSAN_INTEGRATION existed but CI never set it, so every path touching real embedding rows and real KV ran uninstrumented. Now on. ASan stays off there deliberately: libllama is not instrumented. Unit 267/267; multimodal integration 4/4 (62 assertions).

Opening now matches the repo description — prefix sharing turned into Git-style trees — instead of the older "Covalent Inference" line. Gives a reader the mental model in one pass: a KV cache already holds what the model has read, and branching is what turns it into structure. The operations map onto Git moves they already know (fork / prune / pruneSubtree / retainOnly), then two claims that make it more than an analogy — the tree is batched rather than walked, so width is close to free; and a prefix is not only text, so an image encoded once serves N branches. Closes with why that combination matters and where to read next. Anchors verified to resolve.

Renames the column to "Git command" and fills it with actual commands rather than paraphrases, which makes the two rows Git CANNOT express stand out — and those are the ones worth reading. Hard merge is the agents runtime's fan-in: a turn runs on forks, the chosen result is written back onto the shared line as an assistant turn via ctx.extendSpine(userContent, agent.result) (orchestrators.ts:150), and the next generation forks from there. The divergence is discarded; its outcome survives as an ordinary prefix. That is git merge --squash. Soft merge has no Git analogue at all, which is the point. merge_logits accumulates in log-probability space — dst.logits[t] += alpha * sum_i experts[i].logits[t] — so several KV histories of one model shape a single branch's next token while each keeps its own state. Contrastive decoding (DExperts-style) as a tree operation: positive alpha pulls toward the experts, negative pushes away (the implementation is a plain multiply with no clamp, so both signs are meaningful). No dispatch, no KV write. Also de-duplicated the transition that followed: the merge prose already argues the analogy holds, so the next paragraph now moves on to why the tree is cheap rather than restating that it is more than an analogy.

Five corrections to the merge section. The merge rows now name code like every other row. Hard merge is decode_scatter() onto the parent; soft merge is merge_logits(dst, experts, alpha). Previously "fan-in" pointed at nothing. Dropped the decode_each() row and its claim that Git "never advances every branch at once" — that is false. git fetch advances every tracking ref and git rebase --update-refs advances a whole stack. The batching property is real but it is about ONE dispatch, not about Git's inability, so it belongs in the prose below rather than forced into a mapping. Removed the HDK vocabulary. The previous text described an agents runtime writing an assistant turn via ctx.extendSpine — none of which exists at this layer. liblloyal has branches, positions and tokens; the HDK composes the rest on top. Hard merge is now stated as what it is here: the child's KV is discarded and its output re-decoded onto the parent, which is why it costs the tokens twice and yields a shared prefix. Added rebase, which composes today: fork() the new base, decode_scatter() the same tokens onto it. A branch stores its position and never its content, so the caller supplies them — deliberate rather than missing, since content survives a context restart and a position does not. Compressed the whole thing from three dense paragraphs to two, which was the readability complaint.

The last pass answered a presentation complaint by deleting content — the display-math block and the DExperts framing both went, which were the two things worth keeping, and the prose was no shorter for it. Math is a fenced block again, because breaking the page is the point of it. DExperts stays. Compression comes from the prose instead: three paragraphs of 152 words down to 102, each beat now a bold lead word and one or two short sentences (Hard / Soft / Rebase) rather than a run-on.

Copilot

🟡 Changes recommended

A new unit test has an incorrect exact exception expectation and currently fails.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

tests/branch_test.cpp:1186

The PR description’s follow-up says the stub tier has no decode::embd / decode_segments cases, but this section adds both (and the multimodal integration file now contains five cases rather than the stated four). Update the description and reported case/assertion totals so reviewers and release notes reflect the current diff.

TEST_CASE("branch: decode_embd advances position by n_pos, cells by n_rows") {

Files reviewed: 13/14 changed files
Comments generated: 2
Review effort level: Balanced

+ * The rc is the classification a caller acts on (llama.h): `1` = no KV slot,
+ * state restored — the branch is intact; `-1` = invalid batch, state
+ * restored; `2` = aborted and `< -1` = fatal — partial ubatches REMAIN, the
+ * branch is poisoned. The rc must travel as DATA: the binding catches this
+ * type in C++ and forwards `rc` structurally; the exception itself never
+ * crosses N-API.


…one logits capture Two kernel defects, each behind a test that was red first. UB in logits capture. Seven sites copied the context's logits into the branch snapshot with an assert as the only guard; with n_vocab == 0 that is memcpy into an empty buffer (UBSan on CI's Linux job: branch.hpp:1116, null passed to an argument declared nonnull — glibc's memcpy declaration fires it, Apple libc's never does, which is why it was invisible locally). BranchState::capture_logits is the one capture now, and the check is real: a branch with no vocab is refused, never copied into. Seven copies deleted. The error contract over-promised. decode.hpp said rc == 1 means "branch intact", but llama_decode restores only the call it rejects, and many/embd/decode_scatter chunk — earlier chunks stay committed. A caller re-queuing a cohort on rc 1 would decode landed tokens twice onto advanced positions. DecodeError carries `partial` now, fed by the loops that know: many/embd report n_committed through an optional out-param (return type unchanged), decode_scatter counts landed chunks, and decode_segments folds "an earlier segment landed" in via as_partial(), since a prefill is one operation to its caller. The rule, true at every throw site: intact iff rc == 1 && !partial; anything else, prune and replay. decode_scatter also moves position and cells together as each chunk lands. Before, a partial failure left landed branches advanced but uncharged, so their later prune under-counted the pressure gauge. Tests: a stub knob (decode_fail_on_call) puts the failure on a chosen call; TestStore resets the stub, and the four cases that passed on leaked state now set their own preconditions after the fixture (one of them was exercising the UB); no-vocab scatter is refused; later-chunk failures report partial for prefill, decode_scatter, decode_embd and across decode_segments, with the books pinned. 272 cases, ASan+UBSan clean on Linux and macOS.

Copilot

🟡 Changes recommended

Logits destinations must be validated before scatter dispatch to prevent inconsistent KV and branch accounting.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

README.md:77

Like the decode_each example above, this braced initializer list cannot bind to the std::span<const DecodeScatterItem> parameter, so the documented usage fails to compile. Store the items in an array before passing them.

store.decode_scatter({
    {branchA.handle(), system_tokens},  // 200 tokens
    {branchB.handle(), query_tokens},   //  12 tokens
    {branchC.handle(), doc_tokens},     // 800 tokens
});

README.md:65

This example does not compile: decode_each accepts std::span<const DecodeEachItem>, and a braced initializer list cannot be converted to std::span. Materialize an array (or vector) first so users can copy the example successfully.

store.decode_each({{child1.handle(), tok1},
                   {child2.handle(), tok2},
                   {child3.handle(), tok3}});

Files reviewed: 13/14 changed files
Comments generated: 1
Review effort level: Balanced

+      if (!items[i].tokens.empty()) {
+        for (int32_t j = 0; j < i; ++j) {
+          if (items[j].handle == items[i].handle && !items[j].tokens.empty()) {
+            throw std::runtime_error(
+                "BranchStore::decode_scatter - duplicate handle at indices " +
+                std::to_string(j) + " and " + std::to_string(i) +
+                " (sequential calls required for multiple runs per branch)");
+          }
+        }
+      }


…l weights Three follow-ups to dd4667e, each behind a test that was red first. create refuses a model with no vocab. The no-vocab guard lived only in capture_logits, which runs AFTER a chunk has dispatched — an exception there left KV cells at the branch's position with the books unmoved. The invariant every capture stands on is now established where a branch is born: branch::create throws (releasing slot and lease) when llama_vocab_n_tokens is 0, so no decode can ever be dispatched for a branch with nowhere to put its logits. The stub fixture's model has a vocab of 8; the case about the refusal sets 0. rc -1 restores too. DecodeError's rule said intact iff rc == 1 && !partial, but llama.h restores state for -1 (invalid batch) as it does for 1 (no KV slot), and the settle path already prefills a text fallback onto an rc -1 branch. The rule now: INTACT iff the failing call restored state (rc 1 or -1) and nothing before it landed; 1 is a capacity wait, -1 is the input — do not resend the same batch; 2 and < -1 poison. A failed decode on real weights. tests/integration/decode_failure_ integration_test.cpp makes the KV the witness for what the stub suite pins: a unified 256-cell pool and chunks sized so a LATER one cannot find a slot. Five cases — a later scatter chunk (landed children moved and were charged, the refused one did not and still prefills), a chunked prefill that dies mid-way (poisoned, pos_max shows the orphans, prune reclaims them), an oversized item that landed some chunks (partial with an UNMOVED position — the case position alone cannot tell from intact), a segment after a landed one, and an image whose rows outrun the pool through the real projector. Green on CI's own models, CPU.

Copilot

🟡 Changes recommended

Three moderate issues remain in branch cleanup accounting, test setup, and CI coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 14/15 changed files
Comments generated: 3
Review effort level: Balanced

+  if (state->n_vocab <= 0) {
+    // The invariant every logits capture stands on is established here, at
+    // birth: a live branch has somewhere to put its logits. Refusing now
+    // means no decode can ever be dispatched for a branch that does not.
+    s.release(handle);
+    throw std::runtime_error("branch::create - model has no vocab (n_vocab=" +
+                             std::to_string(state->n_vocab) + "); nothing to sample");
+  }


+  llamaStubConfig().n_batch  = 8;
+  llamaStubConfig().n_ubatch = 8;
+
+  TestStore ts(8);
+  TestSamplingParams params;
+  auto* fake_model = reinterpret_cast<llama_model*>(0x2000);
+  BranchHandle h = create(ts.ctx, fake_model, ts.store, 0, params, /*n_batch*/ 8);


… cohort Two of the three paths that batch by handle disagreed about duplicates, and the third re-implemented the rule by hand. decode_scatter refused a repeated handle with a nested scan; decode_each refused nothing, so two items for one branch read the same position and put two tokens on one cell (the new test shows position advancing to 2 over a single cell); and lloyal.node's multimodal cohort, which dispatches one branch at a time so decode_scatter never sees the pair, copied the nested scan — coercing the handles a second time, which is how 5 and 5.5 slipped past it. require_distinct_handles(handles, who) is the rule: one pass with a first-index map, INVALID_HANDLE entries skipped (an empty item occupies no cells and cannot collide). decode_each and decode_scatter call it on the handles they are about to dispatch; the binding calls it on the handles it has marshaled — the values checked are exactly the values dispatched. The nested scans are gone. Quadratic was never the point at n_seq_max width; three statements of one invariant were. Red first: "decode_each refuses a repeated handle before anything is dispatched" — it dispatched, and the position moved twice. 274 cases, ASan+UBSan clean.

Copilot

🔵 Needs a closer look

Unresolved CI, CMake linkage, and test validity issues must be addressed before approval.

Review details

Suppressed comments (11)

Previously missed (4) — in code that hasn't changed since the last review.

README.md:361

This claim is false for the pinned llama.cpp b9581 integration. That version exports llama-common, while this repository's root CMakeLists.txt only recognizes TARGET common; consequently liblloyal::liblloyal does not propagate the common include directories or library needed by headers such as chat_in.hpp. Update the CMake target detection/linking for llama-common (and its base dependency) before documenting it as transitive.
tests/integration/decode_failure_integration_test.cpp:412
This setup fails before exercising decode for any valid projector with n_rows - 8 > 256 - text_before (for example, an image with more than roughly 256 rows), even though the only stated precondition is n_rows > 24. Reserve a fixed 24 cells instead: the first 16-row call lands and the next call is refused for every accepted row count.

This issue also appears on line 432 of the same file.
tests/integration/multimodal_integration_test.cpp:267

This assertion cannot verify slack recovery because root is the last live branch: BranchStore::release unconditionally resets cells_used_ to zero when the final slot is freed. Keep an empty branch alive while pruning root, as the stub test does, so an under-subtraction remains observable.

This issue also appears in the following locations of the same file:

line 321
line 465
tests/integration/multimodal_integration_test.cpp:571

The preceding comment correctly notes that rejection depends on projector capability, but this exact-message assertion only accepts the vision-only path. With an audio-capable mtmd context, the helper creates an audio bitmap and MtmdSource instead throws audio input is not supported, making this portability test fail even though rejection is correct. Accept either documented rejection path (or just the exception type).

.github/workflows/tests.yml:281

The new VL decode-failure case is never run in CI: its name is decode failure: ... embedding rail, the regular step has no LLAMA_MMPROJ_MODEL, and this filter selects only names containing multimodal. Include that case in the VL invocation so the real-KV failure behavior is covered.

        ./build_integration/IntegrationRunner --success \
          --test-case="*multimodal*" 2>&1 | tee test_output.txt

README.md:65

This example does not compile: decode_each accepts a std::span<const DecodeEachItem>, and a nested braced initializer list cannot convert to std::span. Materialize a contiguous array (or vector) before calling it so the documented usage is copy/pasteable.

store.decode_each({{child1.handle(), tok1},
                   {child2.handle(), tok2},
                   {child3.handle(), tok3}});

include/lloyal/mtmd.hpp:59

This documents only the default marker, but mtmd_context_params::media_marker is configurable and the implementation correctly counts mtmd_get_marker(ctx_). A consumer following this text with a custom marker would build a prompt that fails tokenization; document the context marker and identify <__media__> as only the default.

 * The prompt carries one media marker (`mtmd_default_marker()`,
 * `"<__media__>"`) per image; mtmd splits it into interleaved text and image
 * chunks. Text chunks come back as ready token ids — never re-tokenized.

tests/branch_test.cpp:1453

TestStore resets the global stub config in its constructor, so both limits assigned above are immediately restored to 512. The test still throws only because create(..., n_batch=8) supplies a separate branch limit; deleting the new llama_n_ubatch validation would not fail this test. Configure n_ubatch after constructing the fixture and keep the branch batch large enough to isolate the micro-batch check.

  llamaStubConfig().n_batch  = 8;
  llamaStubConfig().n_ubatch = 8;

  TestStore ts(8);

tests/integration/decode_failure_integration_test.cpp:432

This assertion is invalid for the M-RoPE tier. MtmdSource::positions keeps the primary t position at the image base for every row, and llama.cpp's KV pos_max tracks that primary position, so 16 committed rows leave pos_max == base, not base + 15. Check for the image base to distinguish committed image rows from the preceding text.

  CHECK(kv::pos_max(ctx, seq) >= fill + text_before + 16 - 1);

tests/integration/multimodal_integration_test.cpp:323

After retainOnly, the winner is the only live branch, so pruning it triggers the unconditional last-branch pressure reset and this check passes even if inherited slack was never promoted. Create an empty keeper after promotion and leave it live during the winner's prune to make the accounting assertion meaningful.

  prune(winner, store);
  CHECK_MESSAGE(store.kv_pressure().cells_used == 0,
                "retainOnly promotes inherited slack to the winner's own");

tests/integration/multimodal_integration_test.cpp:467

This final slack assertion is also masked by last-branch cleanup: after all children are pruned, releasing spine forces the counter to zero regardless of how many image cells were subtracted. Keep an empty branch live during this prune so the test actually detects stranded image slack.

  prune(spine, store);
  CHECK_MESSAGE(store.kv_pressure().cells_used == 0,
                "releasing the spine recovers the image, slack included");

Files reviewed: 14/15 changed files
Comments generated: 0 new
Review effort level: Balanced

The distinct-handles rule (261200d) had a stub witness only. This case runs it against the KV: decode_each and decode_scatter each refuse a repeated handle before anything is dispatched — position, pos_max and the cell gauge all unmoved — an empty span beside a real one is not a repeat, and the branch keeps working afterwards. Red first, against cee612d: decode_each did not throw; the branch's position went from 1 to 3 while the sequence's max position went from 0 to 1 — two tokens on one cell, the books two ahead of the KV.

Copilot

🔵 Needs a closer look

Unresolved branch accounting, test setup, and CI coverage issues must be fixed before approval.

Review details

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

tests/integration/decode_failure_integration_test.cpp:427

This VL-only test is never exercised in CI: the normal integration step does not set LLAMA_MMPROJ_MODEL, while the dedicated VL step selects only names matching *multimodal*. Include this case in that filter (for example, by naming it consistently with the other multimodal cases) so the real embedding-rail failure path actually runs.

.github/workflows/tests.yml:281

The new VL decode-failure case is named decode failure: ... embedding rail, so it does not match this *multimodal* filter. In the normal integration step LLAMA_MMPROJ_MODEL is unset and REQUIRE_VL() returns immediately, meaning that case never exercises its assertions in CI. Include the embedding-rail failure case in this VL invocation.

        ./build_integration/IntegrationRunner --success \
          --test-case="*multimodal*" 2>&1 | tee test_output.txt

include/lloyal/branch.hpp:1664

release() subtracts position - fork_head from the store's pressure counter. Because position has already been set to start_pos even though this branch decoded nothing, rejecting a vocab-less model with a nonzero start position can subtract another live branch's cells. Validate before stamping the position (and retain the rejected value locally, since release() resets n_vocab).

  if (state->n_vocab <= 0) {
    // The invariant every logits capture stands on is established here, at
    // birth: a live branch has somewhere to put its logits. Refusing now
    // means no decode can ever be dispatched for a branch that does not.
    s.release(handle);
    throw std::runtime_error("branch::create - model has no vocab (n_vocab=" +
                             std::to_string(state->n_vocab) + "); nothing to sample");

include/lloyal/mtmd.hpp:59

The prompt must use the marker configured on this particular mtmd context, not necessarily mtmd_default_marker(): mtmd_context_params.media_marker is configurable and mtmd_get_marker(ctx) exposes the active value. A consumer following this text with a custom marker will get a false marker/image mismatch.

 * The prompt carries one media marker (`mtmd_default_marker()`,
 * `"<__media__>"`) per image; mtmd splits it into interleaved text and image
 * chunks. Text chunks come back as ready token ids — never re-tokenized.

tests/branch_test.cpp:1453

TestStore calls resetStubConfig() in its constructor, so these two overrides are immediately reset to 512. The oversized non-causal call will therefore be accepted instead of exercising the rejection path, causing this test to fail. Construct the fixture before applying the overrides.

  llamaStubConfig().n_batch  = 8;
  llamaStubConfig().n_ubatch = 8;

  TestStore ts(8);

Files reviewed: 14/15 changed files
Comments generated: 0 new
Review effort level: Balanced

…ly tested; CI runs the embedding-rail failure case Three review findings on 18c661e, each behind a test that was red first. A refused create touched the books. The no-vocab check ran after the slot had been stamped with start_pos, so its unwind through release() subtracted a range this never-created branch never decoded: with a live sibling at 10 cells and start_pos 7, the gauge read 3. The vocab is probed BEFORE anything is allocated now — no slot, no lease, nothing for release() to subtract. Test: the gauge and the lease count are exactly where they were. The non-causal test proved the wrong path. It configured the stub's n_batch/n_ubatch before the fixture, and the fixture's reset (R0.2) put both back to 512; the block was refused only because the branch's own n_batch was 8, so llama_n_ubatch's rejection was never exercised. The stub is configured after the fixture and the block fits the batch: only the micro-batch refuses it. CI never ran the embedding-rail failure case: it needs the projector, the plain run has none, and the VL run selected "*multimodal*" only. The VL run selects it by name now; the filter is verified locally against the SmolVLM pair with LLOYAL_VL_STRICT=0 (6 cases, 90 assertions).

Copilot

🟡 Changes recommended

The missing context batch-limit validation is critical, and several integration tests require portability fixes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (2) — in code that hasn't changed since the last review.

tests/integration/decode_failure_integration_test.cpp:493

This KV witness assumes one-dimensional positions advance for every landed embedding row. On an M-RoPE projector, the pinned mtmd implementation assigns the same temporal position (base) to all image rows while x/y vary, so after 16 committed rows pos_max need not reach base + 15; the test therefore fails on the M-RoPE tier it claims to support. Check that image decoding advanced beyond the preceding text position instead.
tests/integration/multimodal_integration_test.cpp:571
This exact-message assertion only passes for a vision-only projector. With an audio-capable mtmd context, the WAV is decoded successfully and the constructor's chunk scan rejects it with MtmdSource - audio input is not supported, so a valid supported projector makes this test fail even though the intended rejection occurs. Assert the exception type (or accept both documented messages).

README.md:70

decode::bin_pack does not perform greedy/first-fit bin packing; it only appends items in input order to the current chunk (decode.hpp:829-849). Calling this greedy can lead users to expect near-minimal dispatch counts that the implementation does not provide.

`decode_each` is one token per branch. `decode_scatter` takes **variable-length** runs and greedy bin-packs them to fill `n_batch`:

include/lloyal/decode.hpp:672

“Rows landed before return” is not guaranteed for rc == 2 or rc < -1: llama.cpp documents that the failing call may retain processed ubatches, while this output is set only to processed from earlier successful calls. Describe this as rows committed by earlier successful dispatches so callers do not mistake it for the actual KV extent after a fatal/aborted call.

 * @param n_committed Optional out: rows landed before return — `n_rows` on
 *        success, fewer when a later chunk failed (see DecodeError::partial)

include/lloyal/decode.hpp:171

“Tokens landed before return” overstates this output for rc == 2 or rc < -1: the failing llama_decode may leave processed ubatches in KV, but n_committed only contains tokens from earlier successful calls. Clarify that distinction so callers do not use this value as the post-failure KV boundary.

 * @param n_committed Optional out: tokens landed before return — `n_tokens`
 *        on success, fewer when a later chunk failed (see DecodeError::partial)

tests/stubs/llama_stubs.h:244

This comment still says failed decodes are rolled back, but the current contract deliberately leaves partial writes in KV and requires pruning; no test reads these tracking fields. Please describe these as generic sequence-removal tracking (or remove the unused tracking in both stub files) so the stub does not advertise a rollback guarantee that no longer exists.

    // Sequence removal tracking — mirrors the seq_cp fields below, so a test
    // can assert that a failed decode rolled back exactly what it wrote.

Files reviewed: 14/15 changed files
Comments generated: 2
Review effort level: Balanced

+  if (item.non_causal) {
+    const int32_t n_ubatch = static_cast<int32_t>(llama_n_ubatch(ctx));
+    if (n > n_batch || n > n_ubatch) {
+      throw std::runtime_error(
+          "decode::embd - non-causal block of " + std::to_string(n) +
+          " rows exceeds n_batch (" + std::to_string(n_batch) +
+          ") or n_ubatch (" + std::to_string(n_ubatch) +
+          "); a bidirectional image must decode in a single dispatch");
+    }
+  }


+  // Leave the image exactly (n_rows - 8) free cells: its first 16-row chunk
+  // lands, a later one cannot. The filler rides the branch's own n_batch.
+  const int32_t free_for_rows = n_rows - 8;
+  const int32_t fill = 256 - text_before - free_for_rows;
+  REQUIRE(fill > 0);
+
+  BranchHandle h = create(ctx, model.get(), store, 0, params, 16);
+  REQUIRE(h != INVALID_HANDLE);
+  const llama_seq_id seq = store.get(h)->seq_id;
+  const auto pad = filler(fill, 1000, n_vocab);
+  prefill(h, pad.data(), pad.size(), store);
+  REQUIRE(get_position(h, store) == fill);


Copilot

🟡 Changes recommended

Two moderate issues remain in constructor validation and M-RoPE-compatible failure testing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

include/lloyal/mtmd.hpp:30

The general claim that liblloyal “does not link anything” contradicts the root CMakeLists.txt, where liblloyal links common and llama transitively, and the README explicitly documents that behavior. Only mtmd is intentionally not linked on the consumer's behalf; narrow this build requirement accordingly so consumers do not receive conflicting integration guidance.

 * **Build requirement (the consumer's, not liblloyal's).** liblloyal is
 * header-only and does not link anything on your behalf — linking is the
 * binding layer's job. A target that includes this header must itself link
 * llama.cpp's `mtmd` target, which PUBLIC-propagates the `tools/mtmd`
 * include path:

tests/branch_test.cpp:879

This rule contradicts DecodeError's public contract in include/lloyal/decode.hpp:95-99: rc == -1 also restores the failing call, so it is intact when no earlier chunk landed. Keeping the test commentary aligned avoids teaching callers to prune an intact invalid-batch failure.

// intact ⇔ rc == 1 && !partial — anything else ⇒ prune and replay.

tests/stubs/llama_stubs.h:244

This comment still describes the rollback test that was removed: failed embedding decodes are now deliberately treated as poisoned because partial seq_rm cannot restore recurrent state. The fields only record calls to llama_memory_seq_rm, so describing them as generic removal tracking avoids documenting behavior the current implementation does not provide.

    // Sequence removal tracking — mirrors the seq_cp fields below, so a test
    // can assert that a failed decode rolled back exactly what it wrote.

Files reviewed: 14/15 changed files
Comments generated: 2
Review effort level: Balanced

+      : ctx_(ctx), sep_(sep), n_embd_inp_(n_embd_inp) {
+    if (!ctx_) {
+      throw std::runtime_error("MtmdSource - NULL mtmd context");
+    }


+  // KV sits ahead of the branch — poisoned, and prune reclaims it.
+  CHECK(get_position(h, store) == fill + text_before);
+  CHECK(store.kv_pressure().cells_used == static_cast<uint32_t>(fill + text_before));
+  CHECK(kv::pos_max(ctx, seq) >= fill + text_before + 16 - 1);


… is built; the embedding-rail case fits every projector Two review findings on f1f8b01, each tested before it was believed. The comment on the non-causal guard was wrong where it pointed and right where it did not look. The pin clamps n_ubatch to n_batch at context creation, so the guard's n_ubatch check already subsumes the context's batch limit for a non-causal block. But the pin also asserts n_tokens <= n_batch inside llama_decode — an abort, not an rc — and every causal chunk loop sizes its chunks from the CALLER's n_batch (a branch's, a free function's argument), not the context's. On real weights, a branch created with n_batch 64 on a context whose batch is 32 took the process down with SIGABRT on its first prefill. require_fits_batch(ctx, n, who) now runs where each batch is built — many, each, scatter, embd — so an oversize chunk is a clear error, never a dead process. Stub tests pin all four primitives; the real-weights case pins the branch path, then shows a fitting chunk still lands. The fixture comment was right: on a dynamic-resolution projector (the Qwen3.5 pair, 576 cells for the fixture) `fill` went negative and the embedding-rail case died at its own precondition. Padding is now optional — none when the image alone outruns the pool — and the witness that rows landed is the POOL rather than pos_max, because an M-RoPE projector holds every row at one temporal position: a prefill sized to the books' idea of free space fails while the orphans stand and lands once prune reclaims them. Green on the SmolVLM pair (CI's, under CI's filter and flag) and on the Qwen3.5 pair.

Copilot

🔵 Needs a closer look

Moderate issues remain in branch failure messaging and tests that incorrectly classify restored decode failures as poisoned.

Review details

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

tests/branch_test.cpp:1769

This does not exercise a poisoned decode: 16 rows fit in the branch's default 512-row batch, so decode_result = -1 fails the first and only call, and llama restores memory for -1. No earlier chunk or orphan can exist, yet the test locks in the unconditional “poisoned” message. Make a later chunk fail by using a smaller branch batch, or use a fatal/aborted return code if the intended case is failure within one call.

include/lloyal/branch.hpp:1266

This message always says the branch is poisoned, but when rc is 1 or -1 and committed == 0, the pinned llama API guarantees that the failing call restored memory; DecodeError itself documents that case as intact. A first-chunk capacity failure will therefore tell callers to discard a reusable branch. Use a neutral message (or condition the poison guidance on the rc/partial intact rule), and align the surrounding recovery documentation with that rule.

      throw decode::DecodeError(rc, committed > 0,
          "BranchStore::decode_embd - llama_decode failed; this branch is "
          "poisoned, prune it and replay onto a fresh one");

include/lloyal/mtmd.hpp:29

This unqualified statement is incorrect for the provided CMake target: liblloyal::liblloyal already links llama and, when available, common transitively (CMakeLists.txt:69-80). Only the optional mtmd dependency is left to consumers, so scope the statement accordingly to avoid contradicting the README and build configuration.

 * **Build requirement (the consumer's, not liblloyal's).** liblloyal is
 * header-only and does not link anything on your behalf — linking is the
 * binding layer's job. A target that includes this header must itself link
 * llama.cpp's `mtmd` target, which PUBLIC-propagates the `tools/mtmd`

tests/branch_test.cpp:879

This invariant omits the other restored-state result: the pinned llama API also restores memory for rc == -1 (invalid input). That contradicts DecodeError's contract above and incorrectly classifies a first-call -1 as poisoned.

// intact ⇔ rc == 1 && !partial — anything else ⇒ prune and replay.

tests/stubs/llama_stubs.h:248

These fields and their implementation writes are now unused, and the comment claims failed decodes are rolled back even though this PR deliberately uses poison-and-prune because partial range removal is unsafe on recurrent models. Remove the obsolete tracking or add a test for a still-supported sequence-removal path so the stubs do not encode the reverted rollback design.

    // Sequence removal tracking — mirrors the seq_cp fields below, so a test
    // can assert that a failed decode rolled back exactly what it wrote.
    bool seq_rm_called = false;
    llama_seq_id seq_rm_seq = -1;
    llama_pos seq_rm_p0 = -1;
    llama_pos seq_rm_p1 = -1;

Files reviewed: 15/16 changed files
Comments generated: 0 new
Review effort level: Balanced

lloyal-research added 2 commits August 30, 2026 09:11

lloyal-research added the skip-gpu-tests label Aug 30, 2026

lloyal-research mentioned this pull request Aug 30, 2026

feat(mtmd): multimodal prefill through the kernel + batched fan-out coverage lloyal-ai/lloyal.node#49

Open

lloyal-research requested a lite review from Copilot August 30, 2026 06:33

Copilot started reviewing on behalf of lloyal-research August 30, 2026 06:33 View session

Copilot AI reviewed Aug 30, 2026

View reviewed changes

lloyal-research removed the skip-gpu-tests label Aug 30, 2026

lloyal-research commented Aug 30, 2026

View reviewed changes

Comment thread include/lloyal/decode.hpp

Comment thread .github/workflows/tests.yml

Comment thread include/lloyal/branch.hpp Outdated

Comment thread include/lloyal/decode.hpp

lloyal-research added 2 commits August 30, 2026 17:25

lloyal-research requested a lite review from Copilot August 30, 2026 07:42

Copilot started reviewing on behalf of lloyal-research August 30, 2026 07:42 View session

Copilot AI reviewed Aug 30, 2026

View reviewed changes

lloyal-research commented Aug 30, 2026

View reviewed changes

Comment thread include/lloyal/decode.hpp

Comment thread include/lloyal/branch.hpp Outdated

Comment thread include/lloyal/mtmd.hpp Outdated

lloyal-research commented Aug 30, 2026

View reviewed changes

lloyal-research requested a balanced review from Copilot August 30, 2026 08:34

Copilot started reviewing on behalf of lloyal-research August 30, 2026 08:34 View session

Copilot AI reviewed Aug 30, 2026

View reviewed changes

lloyal-research added 9 commits August 30, 2026 18:47

lloyal-research requested a balanced review from Copilot September 3, 2026 08:15

Copilot started reviewing on behalf of lloyal-research September 3, 2026 08:15 View session