Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete backend‑neutral inference surface: contracts, provider wire adapters (OpenAI/Anthropic/Ollama), parsers/thinking, scheduler, state stores (in‑memory + filestore) and agent‑memory, discovery/GGUF reader, evaluation/bench/decode tooling, quant modules (codebook/JANG), probes, extensive docs, tests, EUPL‑1.2 licence, and a submodule pointer update. ChangesSingle cohesive cohort
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (35)
docs/inference/gguf.md-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the canonical SPDX header key to avoid licence-scanner misses.
SPDX-Licence-Identifiershould beSPDX-License-Identifier. Please apply this across all newly added docs headers in this PR.Proposed fix
-<!-- SPDX-Licence-Identifier: EUPL-1.2 --> +<!-- SPDX-License-Identifier: EUPL-1.2 -->🤖 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 `@docs/inference/gguf.md` at line 1, The SPDX header in the new docs uses the misspelled key "SPDX-Licence-Identifier"; update all occurrences to the canonical "SPDX-License-Identifier" (replace the string literal "SPDX-Licence-Identifier" with "SPDX-License-Identifier") across the newly added documentation headers in this PR so the licence-scanner recognizes them.docs/ollama/ollama.md-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the canonical SPDX header key to keep licence scanners working.
SPDX-Licence-Identifieris not the recognised SPDX key. Please useSPDX-License-Identifierexactly.Suggested fix
-<!-- SPDX-Licence-Identifier: EUPL-1.2 --> +<!-- SPDX-License-Identifier: EUPL-1.2 -->🤖 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 `@docs/ollama/ollama.md` at line 1, Replace the incorrect SPDX header key `SPDX-Licence-Identifier` with the canonical `SPDX-License-Identifier` in the file (update the header comment string exactly), ensuring the SPDX header uses the exact spelling so licence scanners recognize it.docs/state/project_seed.md-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCorrect SPDX header token for this document.
Please switch to
SPDX-License-Identifier; the current token is non-standard and may be ignored by compliance tooling.Proposed fix
-<!-- SPDX-Licence-Identifier: EUPL-1.2 --> +<!-- SPDX-License-Identifier: EUPL-1.2 -->🤖 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 `@docs/state/project_seed.md` at line 1, Update the SPDX header token at the top of docs/state/project_seed.md by replacing the non-standard "SPDX-Licence-Identifier" token with the correct "SPDX-License-Identifier" token so compliance tools recognize the license header; locate the existing header comment and change the token text only, preserving the same license value and surrounding comment markers.docs/state/memory.md-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix SPDX tag spelling to keep licence scanning reliable.
The header must use
SPDX-License-Identifier(notSPDX-Licence-Identifier) for standards-compliant tooling.Proposed fix
-<!-- SPDX-Licence-Identifier: EUPL-1.2 --> +<!-- SPDX-License-Identifier: EUPL-1.2 -->🤖 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 `@docs/state/memory.md` at line 1, The file contains a misspelled SPDX header "SPDX-Licence-Identifier" which breaks license scanners; locate the header string "SPDX-Licence-Identifier" in docs/state/memory.md and replace it with the correct token "SPDX-License-Identifier" so tooling recognizes the license.docs/state/identity.md-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the exact SPDX header token (
SPDX-License-Identifier).
SPDX-Licence-Identifieris not a valid SPDX tag. Automated licence tooling usually requiresSPDX-License-Identifierexactly, so this may be missed by scanners.Proposed fix
-<!-- SPDX-Licence-Identifier: EUPL-1.2 --> +<!-- SPDX-License-Identifier: EUPL-1.2 -->🤖 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 `@docs/state/identity.md` at line 1, Replace the incorrect SPDX header token `SPDX-Licence-Identifier` with the exact required token `SPDX-License-Identifier` in the file (update the header comment so automated license tooling recognizes it).docs/state/store.md-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStandardise SPDX header spelling in this file as well.
Use
SPDX-License-Identifierexactly to ensure licence scanners recognise the tag.Proposed fix
-<!-- SPDX-Licence-Identifier: EUPL-1.2 --> +<!-- SPDX-License-Identifier: EUPL-1.2 -->🤖 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 `@docs/state/store.md` at line 1, Replace the incorrect SPDX header text "<!-- SPDX-Licence-Identifier: EUPL-1.2 -->" with the standard tag "<!-- SPDX-License-Identifier: EUPL-1.2 -->" so licence scanners recognise it; update the header in docs/state/store.md (look for the existing comment string) to use "License" instead of "Licence".go/bench/bench.go-395-411 (1)
395-411:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
normalizeConfigskipsCachePromptfallback for zero config.When
configZero(cfg)is true, Line 398 returns immediately, soCachePromptnever inheritsPrompt. That can drive prompt-cache benches with an empty cache prompt.Proposed fix
func normalizeConfig(cfg Config) Config { def := DefaultConfig() if configZero(cfg) { - return def + cfg = def } if cfg.Prompt == "" { cfg.Prompt = def.Prompt } @@ if cfg.CachePrompt == "" { cfg.CachePrompt = cfg.Prompt }🤖 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 `@go/bench/bench.go` around lines 395 - 411, normalizeConfig currently returns early when configZero(cfg) is true, which prevents CachePrompt from inheriting Prompt and leaves CachePrompt empty; modify normalizeConfig (function normalizeConfig and call to configZero) so that even when configZero(cfg) is true you still set cfg.CachePrompt = cfg.Prompt (or the default prompt) before returning: call DefaultConfig() as def, and if configZero(cfg) is true assign cfg.Prompt = def.Prompt and cfg.CachePrompt = cfg.Prompt (or set CachePrompt to def.Prompt if you prefer) then return cfg; ensure other fallbacks (MaxTokens, Runs) remain unchanged.go/dataset.go-172-174 (1)
172-174:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Evaluator.Evaluateshould returncore.Result, noterror.This public failure-capable production contract currently exposes
(*EvalReport, error), which breaks the package-widecore.Resulterror-handling model.As per coding guidelines, "
Public production functions that can fail must returncore.Result; callers must branch onr.OKand user.Valueonly after success".🤖 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 `@go/dataset.go` around lines 172 - 174, The Evaluator interface currently exposes Evaluate(ctx context.Context, dataset DatasetStream, cfg EvalConfig) (*EvalReport, error) which violates the package-wide error-handling model; change the signature of Evaluator.Evaluate to return core.Result where Result.Value will hold the *EvalReport on success. Update the Evaluator interface declaration (Evaluator.Evaluate), all concrete implementations, and every call site to construct and return core.Result (set r.OK/r.Value on success or r with error information on failure) and to branch on r.OK before using r.Value; ensure types remain DatasetStream, EvalConfig and that the returned Value wraps *EvalReport.go/decode/example_test.go-9-32 (1)
9-32: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace placeholder examples with real API usage examples.
These examples currently print labels only; they should exercise the corresponding public decode symbols so the example file documents actual behaviour.
As per coding guidelines, "
Public symbols in.gomust have triplet tests in_test.goand usage examples in_example_test.go``".🤖 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 `@go/decode/example_test.go` around lines 9 - 32, The file's example functions currently only print labels; replace each placeholder Example* (ExampleSpeculative, ExamplePromptLookup, ExampleTokenEqual, ExampleTokensText, ExampleCloneTokens) with real usage that calls the corresponding public decode API (construct necessary inputs, call the public functions/types from package decode such as Speculative, PromptLookup, TokenEqual, TokensText, CloneTokens) and print their real return values or observable effects; ensure each example imports and uses the actual symbols, exercises typical inputs, and includes the correct "// Output: ..." comment showing the expected output so `go test` example checks pass.go/dataset_test.go-5-8 (1)
5-8: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
testifyassertions andassert.InDeltafor float checks.This file currently relies on custom checks and direct float equality; please switch to
require/assert, and useassert.InDeltafor floating-point comparisons.As per coding guidelines, "
**/*_test.go: Usetestify/assertfor general checks andtestify/requirefor preconditions in tests" and "Useassert.InDeltafor float comparisons in tests".Also applies to: 130-133
🤖 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 `@go/dataset_test.go` around lines 5 - 8, Replace custom checks and direct float equality in dataset_test.go tests with testify; add imports "github.com/stretchr/testify/assert" and "github.com/stretchr/testify/require" to the import block, use require.* for test preconditions (e.g., setup results) and assert.InDelta for all floating-point comparisons instead of == or manual delta checks; update assertions inside the test functions (look for any float comparisons and calls like t.Fatal/t.Errorf or manual if checks) to use assert.InDelta(actual, expected, delta) and require.NoError/require.NotNil where appropriate.go/decode/decode.go-115-116 (1)
115-116:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPublic decode APIs should follow
core.Resultfailure semantics.
SpeculativeandPromptLookupare public production entry points that can fail, but currently return Goerrortuples instead ofcore.Result.As per coding guidelines, "
Public production functions that can fail must returncore.Result; callers must branch onr.OKand user.Valueonly after success".Also applies to: 163-164
🤖 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 `@go/decode/decode.go` around lines 115 - 116, The public functions Speculative and PromptLookup should return core.Result instead of an (Result, error) tuple; change their signatures to return core.Result, remove error returns, and convert all failure paths to return a core.Result with OK=false and a suitable error message in Value (or other agreed failure encoding), preserving existing Result semantics (callers must check r.OK). Update any internal helpers called (e.g., places returning errors inside Speculative and PromptLookup) to produce core.Result on failure or propagate errors by wrapping them into a failure core.Result; ensure successful paths return core.Result with OK=true and Value populated. Finally, update all callers/tests to branch on r.OK and access r.Value only after success.go/decode/decode_test.go-12-49 (1)
12-49: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAvoid direct float equality in tests and migrate to
testifyassertions.Line 40 compares
AcceptanceRatewith direct equality; useassert.InDelta, withrequire/assertreplacing manualt.Fatalfchecks in this file.As per coding guidelines, "
**/*_test.go: Usetestify/assertfor general checks andtestify/requirefor preconditions in tests" and "Useassert.InDeltafor float comparisons in tests".🤖 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 `@go/decode/decode_test.go` around lines 12 - 49, Update the TestSpeculative_AcceptsAndRejectsDraftTokens_Good test to use testify's require/assert helpers: replace the precondition checks (e.g., err != nil and Mode checks) with require.* (require.NoError, require.Equal) and replace plain t.Fatalf comparisons with assert.* where appropriate; specifically use assert.InDelta to compare result.Metrics.AcceptanceRate to 2.0/3.0 instead of direct float equality, and convert the other t.Fatalf checks to require or assert calls (for example require.Equal for exact matches and assert.NotZero for durations). Keep the same semantics and reference symbols result.Metrics, AcceptanceRate, TestSpeculative_AcceptsAndRejectsDraftTokens_Good, targetCalls, and draftCalls when making the replacements.go/contracts_test.go-77-225 (1)
77-225: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdopt
testify/assertandtestify/requirein these tests.The file currently uses custom
check*helpers throughout; please switch torequirefor preconditions andassertfor behavioural checks to match the test contract consistently.As per coding guidelines, "
**/*_test.go: Usetestify/assertfor general checks andtestify/requirefor preconditions in tests".🤖 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 `@go/contracts_test.go` around lines 77 - 225, Replace the custom check* helpers with testify's require and assert in the tests: import "github.com/stretchr/testify/require" and "github.com/stretchr/testify/assert", use require.* for preconditions (e.g., replace checkNoError, checkTrue before continuing and type assertion checks in TestContracts_OptionalInterfaces, TestContracts_CacheService, TestContracts_EmbeddingAndRerank, TestContracts_Parsers, TestContracts_ModelPackInspector, TestContracts_AgentMemorySession) and use assert.* for behavioural/verifying checks (e.g., replace checkEqual, checkLen, checkNotNil where the test can continue after a failure) and update calls that reference the helper names (checkTrue, checkNoError, checkEqual, checkLen, checkNotNil) accordingly; ensure each test uses require at the top for mandatory conditions (type assertions and returned err checks) and assert for subsequent assertions.go/eval/eval.go-161-217 (1)
161-217:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAlign failing public API and error text contract with project rules.
RunDatasetis public and can fail, but returnserrorinstead ofcore.Result, and several messages usemlx:/ mixed case (EvaluateBatch,BuildBatches,LoRA). Please adapt this API to returncore.Resultand normalise messages to the requiredinference: lowercase message without trailing periodpattern.As per coding guidelines, "Public production functions that can fail must return
core.Result" and "Error strings must use the format:fmt.Errorf(\"inference: lowercase message without trailing period\")".🤖 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 `@go/eval/eval.go` around lines 161 - 217, RunDataset is a public function that returns error and emits non-conforming messages; change its return type to core.Result (and adjust callers) and replace all core.NewError(...) returns with fmt.Errorf(...) using the project's error format "inference: lowercase message without trailing period"; also normalize all literal message text (e.g., loader/adapter/runner checks for EvaluateBatch, BuildBatches, LoadAdapter and dataset warnings) to lowercase phrases starting with "inference:" (no trailing periods) and ensure the function still sets report fields (Report, ModelInfo, Adapter) before returning appropriate core.Result success values.go/gguf_test.go-22-49 (1)
22-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
testify/assertandtestify/requireinstead of customcheck*helpers in tests.These tests currently rely on custom helpers, but project test policy requires
testify/assertfor assertions andtestify/requirefor preconditions.As per coding guidelines, "Use
testify/assertfor general checks andtestify/requirefor preconditions in tests".Also applies to: 60-87
🤖 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 `@go/gguf_test.go` around lines 22 - 49, Replace the custom test helpers in TestGGUF_ReadGGUFInfo_Bad and TestGGUF_DiscoverModels_Ugly with testify's require/assert: import "github.com/stretchr/testify/require" and "github.com/stretchr/testify/assert", use require.NoError(t, err) / require.Error(t, err) and require.Equal(t, GGUFInfo{}, info) for preconditions, and use assert.Len(t, models, 1) and assert.Equal(t, path, models[0].Path) / assert.Equal(t, "gemma4_text", models[0].ModelType) / assert.Equal(t, "gguf", models[0].Format) for non-fatal checks; replace checkNoError, checkError, checkEqual, checkLen calls accordingly while keeping existing helpers like writeMinimalGGUFAt, DiscoverModels, and ReadGGUFInfo unchanged.go/ollama/ollama.go-111-113 (1)
111-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass Ollama's
-1and-2semantics through to the inference layer.
GenerateOptions()silently drops non-positiveNumPredictvalues. Since Ollama's API uses-1for infinite generation and-2to fill context, these should be forwarded toinference.WithMaxTokens()instead of ignored. Currently,NumPredict ≤ 0falls through without any option, causing the inference layer to use its default 256-token limit—silently losing user intent.The
WithMaxTokens()function accepts negative values; pass allNumPredictvalues through.🤖 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 `@go/ollama/ollama.go` around lines 111 - 113, Update GenerateOptions so it forwards all NumPredict values (including non-positive ones) to inference.WithMaxTokens instead of only when NumPredict > 0: remove the conditional that drops NumPredict ≤ 0 and always append inference.WithMaxTokens(options.NumPredict) to opts (referencing GenerateOptions, options.NumPredict, and inference.WithMaxTokens). This preserves Ollama semantics for -1 and -2 by passing them through to the inference layer.go/openai/services.go-369-390 (1)
369-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound request-body reads in
decodeServiceRequest.This currently performs an unbounded
io.ReadAll, which can be abused to consume memory. Add a max body size viahttp.MaxBytesReader(at handler entry) orio.LimitReaderbefore reading.🤖 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 `@go/openai/services.go` around lines 369 - 390, The decodeServiceRequest function uses an unbounded io.ReadAll on r.Body; change this to enforce a maximum request size by either wrapping the request body with http.MaxBytesReader at handler entry or by using io.LimitReader inside decodeServiceRequest (e.g., wrap r.Body with io.LimitReader(r.Body, MaxRequestBodySize) and define a shared MaxRequestBodySize constant). If the read fails due to exceeding the limit, return an appropriate error (use http.StatusRequestEntityTooLarge / 413) and preserve the existing JSON validation/error handling (resultError, writeError) paths so oversized bodies are rejected safely.go/openai/openai.go-152-166 (1)
152-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a request-body size cap before decoding JSON.
io.ReadAllon an unbounded body allows a client to force large memory allocations. Wrapr.Body/bodywithhttp.MaxBytesReader(handler path) orio.LimitReaderbefore reading.🤖 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 `@go/openai/openai.go` around lines 152 - 166, The DecodeRequest function currently uses io.ReadAll(body) which can allocate unbounded memory; wrap the incoming body with a size limiter before reading (e.g., replace io.ReadAll(body) with io.ReadAll(io.LimitReader(body, maxBodySize))) where maxBodySize is a reasonable constant (e.g., 1<<20 for 1 MiB) and return an explicit error if the read hits the limit (or detect overflow by attempting to read one extra byte); update DecodeRequest and keep calls to core.JSONUnmarshalString and resultError unchanged so the function returns a clear "request body too large" error instead of allowing unbounded allocation.go/openai/responses.go-89-104 (1)
89-104:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAlign
ResponseGenerateOptionswith the publiccore.Resultfailure contract.This public fallible function returns
errordirectly instead of the repository-widecore.Resultpattern required by the coding guidelines for public production functions that can fail.🤖 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 `@go/openai/responses.go` around lines 89 - 104, Change ResponseGenerateOptions to follow the repository-wide core.Result failure contract: update its return type to ([]inference.GenerateOption, core.Result), call GenerateOptions(chatReq) as before but capture its (opts, err) and convert any non-nil err into a core.Result failure (e.g., core.ResultFromError or core.NewFailure) while returning opts and core.Result for success; update all callers of ResponseGenerateOptions to handle core.Result instead of an error.go/openai/openai.go-151-211 (1)
151-211:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConvert public adapter functions to return
core.Result.
DecodeRequest,ValidateRequest,GenerateOptions, andNormalizeStopSequencesare public fallible functions that should returncore.Resultrather than rawerror, following the pattern established in the coding guidelines. The exception forerrorvalues applies only to backend interface method implementations; these are utility functions that adapt wire formats and should follow the standard public API contract.🤖 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 `@go/openai/openai.go` around lines 151 - 211, Change the public adapter functions DecodeRequest, ValidateRequest, GenerateOptions (and NormalizeStopSequences) to return core.Result instead of raw error (and in DecodeRequest the success value should be carried in the Result). For each failure path currently returning an error, wrap the failure in core.E (or the project's standard core error/result constructor) and return that core.Result; for successful returns, return a core.Result representing success and containing the value (e.g., the decoded ChatCompletionRequest or slice of inference.GenerateOption). Update all call sites to handle core.Result (check .OK / extract value) accordingly. Ensure function signatures and doc comments are updated to reflect core.Result usage.go/parser/markers.go-13-16 (1)
13-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGemma channel end marker appears malformed and will miss valid closes.
On Line 13–Line 16, the end delimiter is
"<channel|>", but the channel format here is"<|channel>...". This likely prevents reasoning blocks from closing for those variants.Proposed fix
- {start: "<|channel>thought\n", ends: []string{"<channel|>"}, kind: "thinking"}, - {start: "<|channel>thinking\n", ends: []string{"<channel|>"}, kind: "thinking"}, - {start: "<|channel>reasoning\n", ends: []string{"<channel|>"}, kind: "reasoning"}, - {start: "<|channel>analysis\n", ends: []string{"<channel|>"}, kind: "analysis"}, + {start: "<|channel>thought\n", ends: []string{"<|channel|>"}, kind: "thinking"}, + {start: "<|channel>thinking\n", ends: []string{"<|channel|>"}, kind: "thinking"}, + {start: "<|channel>reasoning\n", ends: []string{"<|channel|>"}, kind: "reasoning"}, + {start: "<|channel>analysis\n", ends: []string{"<|channel|>"}, kind: "analysis"},🤖 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 `@go/parser/markers.go` around lines 13 - 16, The end delimiter for the Gemma "channel" markers is malformed ("<channel|>") and will fail to match the corresponding start token "<|channel>..."; update the ends value for the entries whose start is "<|channel>thought\n", "<|channel>thinking\n", "<|channel>reasoning\n", and "<|channel>analysis\n" to the correct closing string "<|channel|>" so those reasoning/thinking markers properly close.go/parser/reasoning.go-28-35 (1)
28-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReasoning token offsets are shifted to the marker, not the reasoning payload.
On Line 28 and Line 34,
StartTokenusestokenOffsetbefore accounting forlen(marker.start), so offsets for extracted reasoning are misaligned.Proposed fix
- if end < 0 { + segmentStart := tokenOffset + len(marker.start) + if end < 0 { reasoning := trimReasoningText(afterStart) if reasoning != "" { - segments = append(segments, inference.ReasoningSegment{Kind: marker.kind, Text: reasoning, StartToken: tokenOffset}) + segments = append(segments, inference.ReasoningSegment{ + Kind: marker.kind, Text: reasoning, StartToken: segmentStart, + }) } break } reasoning := trimReasoningText(afterStart[:end]) if reasoning != "" { - segments = append(segments, inference.ReasoningSegment{Kind: marker.kind, Text: reasoning, StartToken: tokenOffset, EndToken: tokenOffset + end}) + segments = append(segments, inference.ReasoningSegment{ + Kind: marker.kind, Text: reasoning, StartToken: segmentStart, EndToken: segmentStart + end, + }) }🤖 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 `@go/parser/reasoning.go` around lines 28 - 35, The StartToken/EndToken for appended inference.ReasoningSegment are currently using tokenOffset (in the segments append lines around the uses of marker.kind and trimReasoningText(afterStart[:end])) which ignores the marker.start length; adjust StartToken to tokenOffset + len(marker.start) and EndToken to tokenOffset + len(marker.start) + end (or omit EndToken when using the first append) so the offsets point to the reasoning payload rather than the marker; update both places where segments = append(inference.ReasoningSegment{... StartToken: ...}) are constructed (the block using reasoning := trimReasoningText(afterStart[:end]) and the earlier append) to use these corrected calculations.go/quant/codebook/codebook.go-69-282 (1)
69-282: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAlign public failing APIs with
core.Resultcontract.Several public production functions in this file return Go
errordirectly (ParseProfile,ReadProfile,NewTensorDescriptor,ValidateProfile,ValidateTensorDescriptor,MatVec,ValidateTensorPayload). The repository contract requirescore.Resultfor public failure paths outside backend-local interfaces.As per coding guidelines, "
**/*.go: Public production functions that can fail must returncore.Result; callers must branch onr.OKand user.Valueonly after success".🤖 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 `@go/quant/codebook/codebook.go` around lines 69 - 282, Change the public functions ParseProfile, ReadProfile, NewTensorDescriptor, ValidateProfile, ValidateTensorDescriptor, MatVec, and ValidateTensorPayload to return core.Result instead of Go error/tuple results; ensure all success returns set Result.OK = true and Result.Value to the successful value (e.g., *Profile for ParseProfile/ReadProfile, TensorDescriptor for NewTensorDescriptor, []float32 for MatVec, or nil for pure validators) and all failure paths return Result.OK = false with Result.Value containing the error object; update internal returns in each function (e.g., returns that currently do "return nil, err" or "return err" should become "return core.Result{OK:false, Value:err}") and success returns (e.g., "return &profile, nil") should become "return core.Result{OK:true, Value:&profile}" so callers can branch on r.OK and use r.Value only after success.go/probe.go-160-192 (1)
160-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect
ProbeBus.sinksagainst concurrentAdd/EmitProbeaccess.
ProbeBusmutates and iterates the same slice without synchronisation. IfAddandEmitProberun concurrently, this introduces a data race and unstable fan-out behaviour.Suggested fix
import ( + "sync" ) type ProbeBus struct { + mu sync.RWMutex sinks []ProbeSink } func (b *ProbeBus) Add(sink ProbeSink) { if b == nil || sink == nil { return } + b.mu.Lock() + defer b.mu.Unlock() b.sinks = append(b.sinks, sink) } func (b *ProbeBus) EmitProbe(event ProbeEvent) { if b == nil { return } - for _, sink := range b.sinks { + b.mu.RLock() + sinks := append([]ProbeSink(nil), b.sinks...) + b.mu.RUnlock() + for _, sink := range sinks { if sink == nil { continue } sink.EmitProbe(event) } }🤖 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 `@go/probe.go` around lines 160 - 192, ProbeBus currently mutates and iterates the sinks slice without synchronization, causing data races when Add and EmitProbe run concurrently; add a sync.RWMutex field (e.g., mu) to ProbeBus, take mu.Lock() in Add when appending to sinks, and in EmitProbe take mu.RLock(), copy the b.sinks slice to a local variable, release the RLock, then iterate over the copied slice calling sink.EmitProbe(event) so iteration happens without holding the lock and avoids races and potential deadlocks; update references to ProbeBus.sinks usage in NewProbeBus, Add, and EmitProbe accordingly.go/scheduler/scheduler.go-139-152 (1)
139-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve caller context when delegating cancellation to the base model.
CancelRequestcurrently discards the incoming context and always calls the base withcontext.Background(). That can ignore deadlines/cancellation from upstream control paths.🤖 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 `@go/scheduler/scheduler.go` around lines 139 - 152, CancelRequest is discarding the incoming ctx by calling the base model with context.Background(); preserve and forward the provided context when delegating to the underlying model. In the CancelRequest method, when checking m.base for inference.CancellableModel and calling cancellable.CancelRequest, pass the original ctx parameter instead of context.Background() so upstream deadlines/cancellations propagate to the base model.go/scheduler/scheduler.go-107-119 (1)
107-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate active request IDs before registration.
A caller-supplied ID can overwrite an existing
active[id]entry, which breaks cancellation/visibility for the earlier job. Validate uniqueness (while active) and return an error on collision beforeregister.Also applies to: 348-352
🤖 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 `@go/scheduler/scheduler.go` around lines 107 - 119, The code currently assigns a caller-supplied req.ID and then immediately registers the job with m.register, which allows a new request to overwrite an existing active entry; before calling m.register (both at this site and the other occurrence around lines 348–352), check whether core.Trim(req.ID) != "" and if so verify uniqueness against the scheduler's active map (e.g. m.active or the method that looks up active jobs) under the same mutex used by register; if a collision exists return an error to the caller instead of proceeding, otherwise proceed to set req.ID (or m.nextRequestID()) and then call m.register so existing active jobs cannot be overwritten.go/split.go-166-244 (1)
166-244:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse
core.Resultfor public failure paths in split planning APIs.
PlanModelSliceandValidateSplitInferencePlancurrently returnerror, which diverges from the repository’s public error-handling contract. Please adapt these tocore.Result-based returns for consistency with caller branching expectations.As per coding guidelines, "Public production functions that can fail must return
core.Result; callers must branch onr.OKand user.Valueonly after success".🤖 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 `@go/split.go` around lines 166 - 244, Both PlanModelSlice and ValidateSplitInferencePlan must use core.Result for public failure paths instead of returning error; change their signatures to return core.Result (e.g., core.Result[ModelSlicePlan] or the project’s Result pattern) and convert every early error return to construct and return a failing core.Result, while successful paths return a successful core.Result wrapping the value. Update all places inside PlanModelSlice (including errors from modelSlicePresetComponents and the custom-components check) to return a failing core.Result with the original error, and in ValidateSplitInferencePlan convert each core.NewError/core.Errorf return into a failing core.Result; ensure the final success path returns an OK core.Result. Also update callers to branch on r.OK and use r.Value only on success.go/scheduler/scheduler.go-62-92 (1)
62-92:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftWorker goroutines have no shutdown path and can leak past
Close().Workers block on
m.queueforever, andClose()only delegates tom.base.Close()without stopping worker loops. Add scheduler-owned shutdown (for example:donechannel +sync.WaitGroup, and close/cancel onClose) so repeated lifecycle usage does not leak goroutines.Also applies to: 273-278, 292-296
🤖 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 `@go/scheduler/scheduler.go` around lines 62 - 92, The worker goroutines started in New leak because they block forever on m.queue and there is no scheduler-owned shutdown; fix by adding shutdown primitives to Model (e.g., add fields done chan struct{} and wg sync.WaitGroup), change the worker spawn loop to a proper for i := 0; i < maxConcurrent; i++ and for each goroutine call m.wg.Add(1) then go m.worker(i) where worker returns and calls m.wg.Done(); modify worker to select on m.queue and case <-m.done to exit cleanly; update Close (method Close on Model) to signal shutdown by closing or cancelling m.done and then calling m.wg.Wait() before delegating to m.base.Close(); ensure no double-close races (create done once in New).go/quant/jang/jang.go-125-337 (1)
125-337:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFailing public APIs should return
core.Resultinstead of rawerror.
ReadConfig,ParseConfig,NewPackedTensorDescriptor,ValidatePackedTensor,DequantizePackedTensor, andPackQuantizedValuesexposeerrorin public production paths. This diverges from the project’s required failure contract and makes caller handling inconsistent with the rest of the stack.As per coding guidelines, "Public production functions that can fail must return
core.Result; callers must branch onr.OKand user.Valueonly after success".🤖 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 `@go/quant/jang/jang.go` around lines 125 - 337, Change the listed public functions to return core.Result instead of raw error or (*T, error): update signatures for ReadConfig, ParseConfig, NewPackedTensorDescriptor, ValidatePackedTensor, DequantizePackedTensor, and PackQuantizedValues to return a core.Result whose OK flag is true on success and Value contains the success payload (e.g. *Info, *PackedTensorDescriptor, []float32, []byte) and false on failure with Value holding the error; replace all direct returns of nil, err or value, nil with appropriate core.Result values and update their callers (e.g. finalize, BuildPackedProfile, callers of ParseConfig/ReadConfig/NewPackedTensorDescriptor/etc.) to branch on r.OK and use r.Value after success.go/state/memory.go-37-50 (1)
37-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winManifest URIs are dropped, breaking
ResolveURIfor preloaded refs.
NewInMemoryStoreWithManifeststores refs but does not populateurisfromref.URI, so any URI supplied via manifest cannot be resolved later.Suggested fix
func NewInMemoryStoreWithManifest(chunks map[int]string, refs map[int]ChunkRef) *InMemoryStore { copyMap := make(map[int]string, len(chunks)) nextID := 1 @@ refMap := make(map[int]ChunkRef, len(copyMap)) + uriMap := make(map[string]int) for id := range copyMap { refMap[id] = ChunkRef{ ChunkID: id, FrameOffset: uint64(id), HasFrameOffset: true, Codec: CodecMemory, } } for id, ref := range refs { ref.ChunkID = id refMap[id] = ref + if ref.URI != "" { + uriMap[ref.URI] = id + } if id >= nextID { nextID = id + 1 } } return &InMemoryStore{ chunks: copyMap, data: make(map[int][]byte), refs: refMap, - uris: make(map[string]int), + uris: uriMap, nextID: nextID, } }Also applies to: 118-135
🤖 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 `@go/state/memory.go` around lines 37 - 50, NewInMemoryStoreWithManifest (and the similar constructor around lines 118-135) populates refs but never fills the InMemoryStore.uris map from each ref's URI, so ResolveURI cannot find preloaded manifest URIs; update both constructors to, when iterating refs (the loop that assigns ref.ChunkID and refMap[id] = ref), also check ref.URI and if non-empty set uris[ref.URI] = id (ensuring you use the same uris map instance returned in the struct literal), so that InMemoryStore.ResolveURI can resolve manifest-supplied URIs for those refs.go/state/filestore/store.go-56-111 (1)
56-111: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftPublic failing API methods should return
core.Result, not(..., error).The exported production surface here still returns Go error pairs across create/open/read/write paths. That diverges from the package-level result contract and makes call-site handling inconsistent.
As per coding guidelines,
**/*.go: "Public production functions that can fail must returncore.Result; callers must branch onr.OKand user.Valueonly after success".Also applies to: 129-149, 151-193, 195-329
go/state/project_seed.go-105-117 (1)
105-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
project_idis not guaranteed when labels/metadata are initially nil.
setProjectLabelexits on nil maps, soWakeRequestand continuationSleepRequestcan silently missproject_idwhen no labels are supplied.Suggested fix
-func setProjectLabel(labels map[string]string, projectID string) { - if labels == nil || projectID == "" { - return - } +func setProjectLabel(labels map[string]string, projectID string) map[string]string { + if projectID == "" { + return labels + } + if labels == nil { + labels = make(map[string]string, 1) + } if labels["project_id"] == "" { labels["project_id"] = projectID } + return labels }- labels := mergeStringMaps(s.Labels, opts.Labels) - setProjectLabel(labels, s.ProjectID) + labels := mergeStringMaps(s.Labels, opts.Labels) + labels = setProjectLabel(labels, s.ProjectID) @@ - metadata := mergeStringMaps(s.Metadata, opts.Metadata) - setProjectLabel(metadata, s.ProjectID) - labels := mergeStringMaps(s.Labels, opts.Labels) - setProjectLabel(labels, s.ProjectID) + metadata := mergeStringMaps(s.Metadata, opts.Metadata) + metadata = setProjectLabel(metadata, s.ProjectID) + labels := mergeStringMaps(s.Labels, opts.Labels) + labels = setProjectLabel(labels, s.ProjectID)Also applies to: 157-160, 291-297
🤖 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 `@go/state/project_seed.go` around lines 105 - 117, The WakeRequest builder can call setProjectLabel on a nil map (via mergeStringMaps when s.Labels and opts.Labels are nil), causing project_id to be omitted; fix by ensuring the labels map is initialized before setProjectLabel is called (e.g., after labels := mergeStringMaps(...), if labels == nil { labels = make(map[string]string) }), and apply the same nil-check/initialization pattern where setProjectLabel is used elsewhere (the analogous constructors/methods referenced in the review such as the SleepRequest builder and the other occurrences that call setProjectLabel).go/state/filestore/store_test.go-18-23 (1)
18-23: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAlign assertions with
testify/require+testify/assert.Please migrate these table and non-table checks to
requirefor setup/preconditions andassertfor expected outcomes to keep test style consistent with repo rules.As per coding guidelines,
**/*_test.go: "Usetestify/assertfor general checks andtestify/requirefor preconditions in tests".Also applies to: 26-35, 41-46, 49-69, 82-97, 104-143, 150-189, 196-223, 228-381
🤖 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 `@go/state/filestore/store_test.go` around lines 18 - 23, Replace t.Fatalf precondition checks with testify/require and move expected-outcome checks to testify/assert: for the Create() error check use require.NoError(t, err) (precondition) and for the Path() comparison use assert.Equal(t, path, store.Path()) (expected outcome). Import "github.com/stretchr/testify/require" and "github.com/stretchr/testify/assert" and apply the same pattern to the other test assertions in this file (lines covering the ranges mentioned) — use require.* for setup/preconditions (e.g., error/creation checks) and assert.* for equality/behavior checks (e.g., Path(), Read/Write results).go/split_test.go-14-23 (1)
14-23: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdopt
testify/assertandtestify/requirein these tests.These checks currently use custom helpers; please switch preconditions to
require.*and behavioural assertions toassert.*to match the repo test contract.As per coding guidelines,
**/*_test.go: "Usetestify/assertfor general checks andtestify/requirefor preconditions in tests".Also applies to: 30-37, 43-49, 58-66, 71-73, 77-89, 94-103
🤖 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 `@go/split_test.go` around lines 14 - 23, Replace the custom test helpers with testify's require/assert: treat the error precondition checkNoError(t, err) as require.NoError(t, err); convert strict preconditions (if any) to require.* and all behavioral checks to assert.*, e.g. replace checkEqual(t, ModelSlicePresetClient, plan.Preset) with assert.Equal(t, ModelSlicePresetClient, plan.Preset), checkTrue/checkFalse with assert.True/assert.False, and keep require.* only for necessary preconditions (err); update imports to include "github.com/stretchr/testify/assert" and "github.com/stretchr/testify/require". Apply the same replacements for the other indicated blocks (lines 30-37, 43-49, 58-66, 71-73, 77-89, 94-103) referencing the same symbols (plan.HasComponent, plan.AttentionLocal, plan.FFNRemoteCandidate, plan.SourcePath, etc.).go/state/store.go-102-183 (1)
102-183: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftPublic failing helpers should return
core.Resultinstead of(value, error).
Resolve,ResolveBytes,ResolveRefBytes, andResolveURIare exported production helpers that can fail, but currently return error pairs. This diverges from the package contract and pushes mixed error semantics onto callers.As per coding guidelines, "Public production functions that can fail must return
core.Result; callers must branch onr.OKand user.Valueonly after success".🤖 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 `@go/state/store.go` around lines 102 - 183, The four exported helpers Resolve, ResolveBytes, ResolveRefBytes, and ResolveURI currently return (Chunk, error) but must return core.Result per package guidelines; change each function signature to return core.Result, on success return a core.Result with OK:true and Value set to the Chunk, and on failure return a core.Result with OK:false and the error (or appropriate error information) set; update all early error returns inside Resolve, ResolveBytes, ResolveRefBytes, and ResolveURI to construct and return a core.Result failure, and ensure the final successful return builds a core.Result success with the Chunk as Value so callers can branch on r.OK and use r.Value only after success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e65b2d7f-2704-419b-a644-5c93b5d4f422
⛔ Files ignored due to path filters (1)
go.workis excluded by!**/*.work
📒 Files selected for processing (98)
LICENCEdocs/README.mddocs/anthropic/anthropic.mddocs/inference/README.mddocs/inference/capability.mddocs/inference/contracts.mddocs/inference/dataset.mddocs/inference/discover.mddocs/inference/gguf.mddocs/inference/identity.mddocs/inference/inference.mddocs/inference/local_tuning.mddocs/inference/options.mddocs/inference/probe.mddocs/inference/service.mddocs/inference/training.mddocs/ollama/ollama.mddocs/openai/README.mddocs/openai/openai.mddocs/openai/responses.mddocs/openai/services.mddocs/state/README.mddocs/state/agent_memory.mddocs/state/filestore.mddocs/state/identity.mddocs/state/memory.mddocs/state/project_seed.mddocs/state/store.mdexternal/gogo/anthropic/anthropic.gogo/anthropic/anthropic_test.gogo/bench/bench.gogo/bench/bench_test.gogo/capability.gogo/capability_example_test.gogo/capability_test.gogo/contracts.gogo/contracts_example_test.gogo/contracts_test.gogo/dataset.gogo/dataset_example_test.gogo/dataset_test.gogo/decode/decode.gogo/decode/decode_test.gogo/decode/example_test.gogo/discover.gogo/eval/eval.gogo/gguf.gogo/gguf_test.gogo/identity.gogo/identity_example_test.gogo/identity_test.gogo/ollama/ollama.gogo/ollama/ollama_test.gogo/openai/openai.gogo/openai/openai_test.gogo/openai/responses.gogo/openai/responses_test.gogo/openai/services.gogo/openai/services_test.gogo/parser/builtin.gogo/parser/markers.gogo/parser/reasoning.gogo/parser/reasoning_test.gogo/parser/registry.gogo/parser/registry_test.gogo/parser/selector.gogo/parser/thinking.gogo/parser/thinking_test.gogo/parser/tools.gogo/parser/tools_test.gogo/parser/types.gogo/probe.gogo/probe_example_test.gogo/probe_test.gogo/quant/codebook/codebook.gogo/quant/codebook/codebook_test.gogo/quant/jang/jang.gogo/quant/jang/jang_test.gogo/scheduler/example_test.gogo/scheduler/scheduler.gogo/scheduler/scheduler_test.gogo/service.gogo/service_test.gogo/split.gogo/split_example_test.gogo/split_test.gogo/state/agent_memory.gogo/state/filestore/store.gogo/state/filestore/store_test.gogo/state/identity.gogo/state/memory.gogo/state/project_seed.gogo/state/project_seed_test.gogo/state/state_test.gogo/state/store.gogo/tuning.gogo/tuning_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
go/bench/bench_test.go (2)
442-442:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename test suffix to match
_Good/_Bad/_Uglyconvention
TestAdapterInfo_IsEmpty_GoodBaddoes not follow the required suffix scheme. Split or rename it to compliant names (for example, separate_Goodand_Badtests).As per coding guidelines: "Tests use the
_Good/_Bad/_Uglysuffix convention:_Goodfor happy path,_Badfor expected error conditions,_Uglyfor edge cases and surprising-but-valid behaviour".🤖 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 `@go/bench/bench_test.go` at line 442, Rename or split the test function TestAdapterInfo_IsEmpty_GoodBad to follow the _Good/_Bad/_Ugly convention: create two tests such as TestAdapterInfo_IsEmpty_Good (covering the happy-path assertions for AdapterInfo.IsEmpty) and TestAdapterInfo_IsEmpty_Bad (covering the expected-failure or invalid input cases), move the relevant assertions from the existing TestAdapterInfo_IsEmpty_GoodBad into the appropriate new test functions, and update any test callers or references accordingly.
46-507:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSwitch assertions to
testify/assert+testify/requireThese tests currently use manual
t.Fatal/t.Fatalfassertions throughout. Please convert torequirefor preconditions andassertfor behavioural checks to match the project test standard.As per coding guidelines: "Use
testify/assertfor general checks andtestify/requirefor preconditions in tests".🤖 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 `@go/bench/bench_test.go` around lines 46 - 507, Replace manual t.Fatal/t.Fatalf checks across the tests to use testify/require for preconditions (e.g., error != nil, nil report, runner setup expectations) and testify/assert for behavioral assertions (value equality/inequality, slices, booleans) so tests follow the project standard; add imports for "github.com/stretchr/testify/assert" and "github.com/stretchr/testify/require" and in each test (e.g., TestRun_AggregatesGenerationSummary_Good, TestRun_FallsBackToElapsedWhenTotalDurationZero_Good, TestRun_RequiresGenerate_Bad, TestRun_PropagatesGenerateError_Bad, TestRun_NilContextDefaultsToBackground_Good, TestRun_PopulatesModelInfoFromCallback_Good, TestRun_DispatchesVerbCallbacksWhenIncludeFlagsSet_Good, TestRun_SkipsVerbCallbacksWhenIncludeFlagsFalse_Good, TestRun_QualityChecks_Good, etc.) convert initial preconditions like err==nil or report==nil to require.NoError/require.NotNil and convert subsequent comparisons to assert.Equal/assert.True/assert.Len/assert.Contains as appropriate while preserving the same failure messages/semantics.go/bench/bench.go (1)
360-415:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReturn
core.ResultfromRuninstead of(*Report, error)
Runis a public production function that can fail, but it currently returns a Goerrorpair. This breaks the project API contract and makes caller handling inconsistent withcore.Result.As per coding guidelines: "Public production functions that can fail must return
core.Result; callers must branch onr.OKand user.Valueonly after success".🤖 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 `@go/bench/bench.go` around lines 360 - 415, Change Run's signature to return core.Result instead of (*Report, error); when an error occurs (e.g., missing runner.Generate or runGeneration error) return a failing core.Result with the error, and on success return a successful core.Result containing the *Report value. Update all return sites in Run (the early core.NewError(...) return and the runGeneration error return, plus the final success return) to construct core.Result values (OK:false with the error for failures, OK:true with Value: report for success). Leave the Report construction and population logic unchanged, and ensure callers of Run now branch on the returned core.Result (check r.OK before using r.Value).go/state/filestore/store.go (1)
57-330:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPublic failing
StoreAPIs should usecore.ResultThe exported store surface uses
(..., error)across many failing methods (Create,Open,Resolve*,Put*, etc.). That diverges from the repository’s required result contract for production APIs and should be normalised.As per coding guidelines: "Public production functions that can fail must return
core.Result; callers must branch onr.OKand user.Valueonly after success".🤖 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 `@go/state/filestore/store.go` around lines 57 - 330, The public store methods (Create, Open, Path, ChunkCount, Close, Get, Resolve, ResolveURI, Put, PutBytes, PutBytesStream, ResolveBytes, ResolveRefBytes) must be changed to return core.Result instead of (..., error): update each function signature to return core.Result whose Value holds the original successful return (e.g., *Store, state.Chunk, state.ChunkRef, string, int, or nil) and on failure return a failing core.Result built from existing error values (use core.E/core.NewError as before); inside these functions replace direct error returns with constructing and returning the appropriate core.Result (OK=false) and on success wrap the return value in a successful core.Result (OK=true/Value=...), and update any internal helper returns (e.g., resolveLocked, resolveBytesLocked, rollbackWriteLocked usage sites) accordingly so callers branch on r.OK and use r.Value.
🧹 Nitpick comments (1)
go/bench/bench_test.go (1)
23-44: ⚡ Quick winUse shared test stubs instead of introducing
newFakeRunner
newFakeRunneradds a parallel local stub pattern. Please reuse the existingstubBackend/stubTextModeltest doubles frominference_test.goto keep test fixtures consistent.As per coding guidelines: "Use existing
stubBackend/stubTextModelfrominference_test.gofor tests rather than creating new stubs".🤖 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 `@go/bench/bench_test.go` around lines 23 - 44, Replace the local test stub newFakeRunner by reusing the existing stubBackend/stubTextModel test doubles: remove newFakeRunner and instead configure stubTextModel (used by stubBackend) to return the desired generation outputs and errors (match generationText, generationMetrics, generationError behavior) so the Runner.Generate behavior is provided by stubTextModel/stubBackend; adapt tests to instantiate stubBackend and set its underlying stubTextModel fields rather than creating a new Runner with a custom Generate closure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@go/bench/bench_test.go`:
- Line 442: Rename or split the test function TestAdapterInfo_IsEmpty_GoodBad to
follow the _Good/_Bad/_Ugly convention: create two tests such as
TestAdapterInfo_IsEmpty_Good (covering the happy-path assertions for
AdapterInfo.IsEmpty) and TestAdapterInfo_IsEmpty_Bad (covering the
expected-failure or invalid input cases), move the relevant assertions from the
existing TestAdapterInfo_IsEmpty_GoodBad into the appropriate new test
functions, and update any test callers or references accordingly.
- Around line 46-507: Replace manual t.Fatal/t.Fatalf checks across the tests to
use testify/require for preconditions (e.g., error != nil, nil report, runner
setup expectations) and testify/assert for behavioral assertions (value
equality/inequality, slices, booleans) so tests follow the project standard; add
imports for "github.com/stretchr/testify/assert" and
"github.com/stretchr/testify/require" and in each test (e.g.,
TestRun_AggregatesGenerationSummary_Good,
TestRun_FallsBackToElapsedWhenTotalDurationZero_Good,
TestRun_RequiresGenerate_Bad, TestRun_PropagatesGenerateError_Bad,
TestRun_NilContextDefaultsToBackground_Good,
TestRun_PopulatesModelInfoFromCallback_Good,
TestRun_DispatchesVerbCallbacksWhenIncludeFlagsSet_Good,
TestRun_SkipsVerbCallbacksWhenIncludeFlagsFalse_Good,
TestRun_QualityChecks_Good, etc.) convert initial preconditions like err==nil or
report==nil to require.NoError/require.NotNil and convert subsequent comparisons
to assert.Equal/assert.True/assert.Len/assert.Contains as appropriate while
preserving the same failure messages/semantics.
In `@go/bench/bench.go`:
- Around line 360-415: Change Run's signature to return core.Result instead of
(*Report, error); when an error occurs (e.g., missing runner.Generate or
runGeneration error) return a failing core.Result with the error, and on success
return a successful core.Result containing the *Report value. Update all return
sites in Run (the early core.NewError(...) return and the runGeneration error
return, plus the final success return) to construct core.Result values (OK:false
with the error for failures, OK:true with Value: report for success). Leave the
Report construction and population logic unchanged, and ensure callers of Run
now branch on the returned core.Result (check r.OK before using r.Value).
In `@go/state/filestore/store.go`:
- Around line 57-330: The public store methods (Create, Open, Path, ChunkCount,
Close, Get, Resolve, ResolveURI, Put, PutBytes, PutBytesStream, ResolveBytes,
ResolveRefBytes) must be changed to return core.Result instead of (..., error):
update each function signature to return core.Result whose Value holds the
original successful return (e.g., *Store, state.Chunk, state.ChunkRef, string,
int, or nil) and on failure return a failing core.Result built from existing
error values (use core.E/core.NewError as before); inside these functions
replace direct error returns with constructing and returning the appropriate
core.Result (OK=false) and on success wrap the return value in a successful
core.Result (OK=true/Value=...), and update any internal helper returns (e.g.,
resolveLocked, resolveBytesLocked, rollbackWriteLocked usage sites) accordingly
so callers branch on r.OK and use r.Value.
---
Nitpick comments:
In `@go/bench/bench_test.go`:
- Around line 23-44: Replace the local test stub newFakeRunner by reusing the
existing stubBackend/stubTextModel test doubles: remove newFakeRunner and
instead configure stubTextModel (used by stubBackend) to return the desired
generation outputs and errors (match generationText, generationMetrics,
generationError behavior) so the Runner.Generate behavior is provided by
stubTextModel/stubBackend; adapt tests to instantiate stubBackend and set its
underlying stubTextModel fields rather than creating a new Runner with a custom
Generate closure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ad806ee-04a8-4ec8-ae4b-4002efc1f092
📒 Files selected for processing (3)
go/bench/bench.gogo/bench/bench_test.gogo/state/filestore/store.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
go/state/identity.go (1)
58-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse UK spelling in comments (
serialisable).Please change “serializable” to “serialisable” for guideline consistency.
As per coding guidelines,
Use UK English spelling (colour, organisation, serialise, licence) in code and comments.🤖 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 `@go/state/identity.go` at line 58, Update the comment above the SamplerConfig declaration to use UK English spelling: replace the word "serializable" with "serialisable" in the comment describing SamplerConfig so it reads "SamplerConfig is the serialisable form of generation sampler settings." Ensure the change targets the comment tied to the SamplerConfig symbol in go/state/identity.go.go/state/store.go (2)
105-186:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPublic fallible helpers should return
core.Resultinstead of(value, error).
Resolve,ResolveBytes,ResolveRefBytes, andResolveURIare exported production functions that can fail, but they currently return Go error pairs rather thancore.Result.As per coding guidelines,
Public production functions that can fail must return core.Result; callers must branch on r.OK and use r.Value only after success.🤖 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 `@go/state/store.go` around lines 105 - 186, The four exported helpers Resolve, ResolveBytes, ResolveRefBytes and ResolveURI must return core.Result instead of (Chunk, error): change their signatures to return core.Result and update all return sites to produce a result with Ok true and Value set to the Chunk on success, or Ok false with Error set to the appropriate error (e.g. &ChunkNotFoundError{...}, &URIChunkNotFoundError{...}, or wrapped errors from resolver.Resolve / resolver.ResolveBytes / resolver.ResolveRefBytes); keep the same nil-context handling and interface checks for Resolver, BinaryResolver, RefBinaryResolver and URIResolver and convert their error returns into failure core.Result values. Ensure callers are updated to inspect r.OK and use r.Value only on success.
13-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStandardise error messages to the required
inference:format.These error strings do not follow the required message format contract; please prefix and keep the message lowercase with no trailing period.
As per coding guidelines,
Error strings must use the format: fmt.Errorf("inference: lowercase message without trailing period").🤖 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 `@go/state/store.go` around lines 13 - 99, Update the error messages to the required "inference:" format: change ErrChunkNotFound to use core.NewError("inference: state chunk not found"); update ChunkNotFoundError.Error() to return core.Sprintf("inference: state chunk %d not found", e.ID); and update URIChunkNotFoundError.Error() to return "inference: state chunk uri not found" when URI is empty and core.Sprintf("inference: state chunk uri %q not found", e.URI) otherwise (use lowercase "uri" and no trailing period). Ensure these changes reference the existing symbols ErrChunkNotFound, ChunkNotFoundError.Error, and URIChunkNotFoundError.Error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@go/state/identity.go`:
- Line 58: Update the comment above the SamplerConfig declaration to use UK
English spelling: replace the word "serializable" with "serialisable" in the
comment describing SamplerConfig so it reads "SamplerConfig is the serialisable
form of generation sampler settings." Ensure the change targets the comment tied
to the SamplerConfig symbol in go/state/identity.go.
In `@go/state/store.go`:
- Around line 105-186: The four exported helpers Resolve, ResolveBytes,
ResolveRefBytes and ResolveURI must return core.Result instead of (Chunk,
error): change their signatures to return core.Result and update all return
sites to produce a result with Ok true and Value set to the Chunk on success, or
Ok false with Error set to the appropriate error (e.g. &ChunkNotFoundError{...},
&URIChunkNotFoundError{...}, or wrapped errors from resolver.Resolve /
resolver.ResolveBytes / resolver.ResolveRefBytes); keep the same nil-context
handling and interface checks for Resolver, BinaryResolver, RefBinaryResolver
and URIResolver and convert their error returns into failure core.Result values.
Ensure callers are updated to inspect r.OK and use r.Value only on success.
- Around line 13-99: Update the error messages to the required "inference:"
format: change ErrChunkNotFound to use core.NewError("inference: state chunk not
found"); update ChunkNotFoundError.Error() to return core.Sprintf("inference:
state chunk %d not found", e.ID); and update URIChunkNotFoundError.Error() to
return "inference: state chunk uri not found" when URI is empty and
core.Sprintf("inference: state chunk uri %q not found", e.URI) otherwise (use
lowercase "uri" and no trailing period). Ensure these changes reference the
existing symbols ErrChunkNotFound, ChunkNotFoundError.Error, and
URIChunkNotFoundError.Error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 406ae289-de22-40c2-a512-f57c6d05db21
📒 Files selected for processing (11)
docs/README.mddocs/ollama/ollama.mddocs/state/README.mddocs/state/agent_memory.mddocs/state/filestore.mddocs/state/identity.mddocs/state/memory.mddocs/state/store.mdgo/state/filestore/store_test.gogo/state/identity.gogo/state/store.go
✅ Files skipped from review due to trivial changes (7)
- docs/state/README.md
- docs/state/memory.md
- docs/README.md
- docs/state/filestore.md
- docs/state/store.md
- docs/state/agent_memory.md
- docs/state/identity.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go/go.mod`:
- Line 5: Remove the non-stdlib dependency by deleting the `require dappco.re/go
v0.10.0` entry from go.mod, remove any corresponding entries from go.sum (or run
`go mod tidy` to clean them), and update all non-test Go files that import core
"dappco.re/go" to either use equivalent stdlib packages or replace the imported
functionality with local code; specifically search for the import alias core
"dappco.re/go" and refactor callers that rely on its APIs to use
standard-library alternatives (e.g., context, net/http, encoding/json, io, os)
or in-repo implementations, then run `go vet`/`go build`/`go test` to verify
compilation.
🪄 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
Run ID: 30067291-0ce1-468b-9a16-f0a373d28259
⛔ Files ignored due to path filters (1)
go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go/gguf.gogo/go.mod
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go/state/filestore/store.go`:
- Around line 231-235: Replace the unsafe type assertion on metaResult.Value
with the existing resultError helper: when core.JSONMarshal(meta) returns
!metaResult.OK, call resultError(metaResult) and return that error along with
the zero-value state.ChunkRef{}; update the block using the symbols metaResult,
core.JSONMarshal, resultError and state.ChunkRef to ensure consistent,
panic-free error extraction.
🪄 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
Run ID: 4edc7b0b-1992-4531-b815-7a94b36b5fc9
📒 Files selected for processing (2)
go/state/filestore/store.gogo/state/filestore/store_bench_test.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| func productionQuantizationPackLock(input productionQuantizationPackLockInput) ProductionQuantizationPackLock { | ||
| tokenizerSHA := "cc8d3a0ce36466ccc1278bf987df5f71db1719b9ca6b4118264f45cb627bfe0f" |
|
|
||
| func productionQuantizationPackLock(input productionQuantizationPackLockInput) ProductionQuantizationPackLock { | ||
| tokenizerSHA := "cc8d3a0ce36466ccc1278bf987df5f71db1719b9ca6b4118264f45cb627bfe0f" | ||
| tokenizerConfigSHA := "90c3a3ba5bf53818383a58e1a776cbcacd2a038d4812eaa373e1522f2d06f3df" |
| if body != nil { | ||
| reader = bytes.NewReader(body) | ||
| } | ||
| upReq, err := http.NewRequestWithContext(c.Request.Context(), c.Request.Method, url, reader) |
| copy(shuffled, responses) | ||
|
|
||
| rng := rand.New(rand.NewSource(seed)) | ||
| rng.Shuffle(len(shuffled), func(i, j int) { |
| // If db is empty, defaults to "training". | ||
| func NewInfluxClient(url, db string) *InfluxClient { | ||
| if url == "" { | ||
| url = "http://10.69.69.165:8181" |
| dir := filepath.Join(os.TempDir(), "lthn-lem", hex.EncodeToString(h.Sum(nil)[:8])) | ||
| mlxDst := filepath.Join(dir, "mlx.metallib") | ||
|
|
||
| if err := os.MkdirAll(dir, 0o755); err != nil { |
| influxURL := envOr("INFLUX_URL", "http://localhost:8181") | ||
| influxDB := envOr("INFLUX_DB", "training") | ||
| apiURL := envOr("LEM_API_URL", "http://localhost:8080") | ||
| m3Host := envOr("M3_HOST", "10.69.69.108") |
…efile
Closes the two things the audit found missing for the linux lanes (the engine +
kernel + nvcc/HIP-CPU compile-tests already survived in engine/hip; only these
were dropped):
- cmd/lem now blank-imports engine/hip alongside engine/metal. Both are
platform-tagged (metal darwin/arm64, hip linux/amd64), so one binary registers
the Apple engine on macOS and the ROCm/CUDA/CPU backend on linux; each is a
no-op stub off-platform. Verified: cmd/lem still builds clean on darwin.
- Makefile (brought from go-rocm, adapted): the lthn-{amd,cuda,cpu-x86,cpu-aarch64}
artifact targets — one cmd/lem source + one go/engine/hip/kernels/rocm_kernels.hip,
differentiated by HIP_PLATFORM (amd/nvidia) + CGO + tags. cuda/cpu were never
separate code, just toolchain selections; this is the selection that didn't
come over.
Follow-up (linux box): the static AMD/CUDA link needs the rocm-hip/rocm-clr/
rocr-runtime submodules (external/*); the cpu lanes build without them.
Co-Authored-By: Virgil <virgil@lethean.io>
…/cuda link The Makefile's static AMD/CUDA lanes (hsa-static-archive / hip-static-archive) need the ROCm C++ runtime sources. go-rocm had these as (later-orphaned) submodules; adding them cleanly to go-inference/external/ at the canonical upstreams, pinned to release/rocm-rel-7.2 (matches the Makefile's ROCM_FALLBACK /opt/rocm-7.2.0): - external/rocm-hip -> github.com/ROCm/HIP - external/rocm-clr -> github.com/ROCm/clr - external/rocr-runtime -> github.com/ROCm/ROCR-Runtime Shallow (build deps, not Go modules — not in go.work); only fetched on the linux box for the amd/cuda static build. macOS/metal clones ignore them. Co-Authored-By: Virgil <virgil@lethean.io>
Ports go-mlx's pkg/score (the lthn.ai/score LEK signal — phonetics,
cmudict, sycophancy, hostility, differential, authority) into go-inference
as eval/score/lek, and wires it into train's score-cascade so the SFT/SSD
scorer hook is no longer a nil no-op. go-inference now scores its own
training runs — the one real feature it was still missing.
- eval/score/lek: faithful copy of the 13-file scorer (+ tests, cmudict
starter data). package score -> lek; self-import + doc-example
qualifiers retargeted; core + i18n/reversal deps unchanged. Scoring
logic, weights and dimensions byte-identical to go-mlx.
- cmd/lem/score.go: lekScoreFunc adapter. The driver owns the scorer and
the train lib stays scorer-neutral (AX-8, lib never imports consumer).
Maps lek.ScorePair -> ScoreRecord{LEK, Tier, Hostility, Echo}, mirroring
go-mlx's original cascade wiring line-for-line.
- train/command.go: thread Score ScoreFunc through SFT/SSD configs; the
honest "no scorer wired" notes now fire only when Score == nil.
- cmd/lem sft/ssd: inject lekScoreFunc() so -score-cascade / -score-samples
score with the real LEK scorer.
Co-Authored-By: Virgil <virgil@lethean.io>
…ide + codecov 95%
The Taskfile had no Go-test targets at all (only the metallib/build chain);
AGENTS.md documented tests but never benchmarks; there was no coverage gate.
- Taskfile: add test (portable, runs anywhere — the CI/codecov lane),
test:metal (engine/metal, -tags metal_runtime, needs task metallib),
cover (-> coverage.out for codecov + prints the total), bench (the
lethean-perf instrument: -run='^$' -bench=. -benchmem -benchtime=20x),
and qa (gofmt check + vet + portable tests).
- AGENTS.md: new "Writing Tests, Examples & Benchmarks" section — the
{file}_test / _example_test / _bench_test trio, the name-the-symbol rule
(a test must exercise the symbol it names, not route through a helper),
AX-7 Good/Bad/Ugly triplets, core.Result assertion shape, and the
benchmark recipe (setup outside the loop, ResetTimer, a package sink so
the call can't be elided, read B/op as hard as allocs/op). Working Locally
now points at the task targets + the coverage gate.
- codecov.yml: 95% on project and patch, measured on the portable
coverage.out surface (external/patches/build/gui ignored).
Co-Authored-By: Virgil <virgil@lethean.io>
…odel Speculative decode assumed one method (a separate assistant draft model). There are other ways to do MTP — EAGLE feature drafters, Medusa parallel heads, in-model MTP heads, n-gram/prompt-lookup — so the drafter now declares its method as a type INFERRED FROM THE MODEL, threaded to the decode path to dispatch on. - model.MTPMethod + MTPDraftModel: the speculative method, declared by the drafter's registered AssistantSpec and stamped onto AssistantConfig by ParseAssistantConfig (resolveMTPMethod defaults unset -> draft-model, the only method shipped and what every legacy checkpoint uses). - gemma4 -assistant declares Method: MTPDraftModel. - AssistantPair.Method() exposes it for the decode driver. - Tests: ParseAssistantConfig stamps the spec's method; unset -> draft-model. Behaviour unchanged (single method today); the seam lets EAGLE/Medusa/etc. each land as a const + a decode branch. Decode dispatch is a follow-on. Co-Authored-By: Virgil <virgil@lethean.io>
MoE gating was hardcoded: the metal router always does softmax top-k, while model/composed/moe.go separately does softmax + norm-topk + shared expert — two hardcoded paths, not a typed dispatch. Mirror the MTP-method seam: the model DECLARES its router gating as a type inferred onto Arch, so the engine applies it rather than assuming softmax (the Arch.AttnScale / EmbedScale discipline). - model.MoEGating + MoEGatingSoftmax: the router expert-scoring method, a new Arch field (resolveMoEGating defaults unset -> softmax, the only variant the metal engine ships and gemma4's method). - gemma4 config.Arch() declares MoEGatingSoftmax when enable_moe_block is set. - Tests: gemma4's MoE arch yields softmax + a dense arch declares none (the end-to-end inference); resolveMoEGating's empty -> softmax default. Behaviour unchanged (softmax only today); sigmoid gating, norm_topk_prob, routed-scaling and shared experts each land as a value + a router branch — model/composed/moe.go is the reference for the second variant. Co-Authored-By: Virgil <virgil@lethean.io>
…th (~34%) The fused gate+up MoE kernel (moeBlockQuantAfterRouter's fusedExperts, ~34% faster than running gate + up as two barriered dispatches) engaged only when a checkpoint shipped a pre-concatenated switch_glu.gate_up_proj. gemma4 26B-A4B ships SEPARATE gate_proj/up_proj, so the fast path was dormant. gemma4 now DECLARES the fusion (Arch.FuseExpertGateUp) and the load synthesises the concatenated ExpGateUp from the separate halves — the same declare-from-the-model seam as Arch.MoEGating / MTPMethod. - model.Arch.FuseExpertGateUp: model-declared opt-in (gemma4 sets it when MoE is on). - load_shared.go moeToQuant: synthesise ExpGateUp (fuseExpertGateUpQuant, promoted from the bench helper, marked QuantWeight.resident) + drop the separate halves when the flag is set and the checkpoint ships them separate. This MATERIALISES the fused experts on the heap, trading the separate weights' safetensors mmap zero-copy for the fused-path speed — which is why it is opt-in per model, not automatic. - QuantWeight.resident + viewQuantWeight: a synthesised weight is a heap buffer, not a mapped-shard view, so the resident setup resident-COPIES it instead of the shard lookup. The shard-view guard (view/view4 -> bufForAligned errors on an unexpected non-shard weight) stays intact for genuine weights — only the flagged synthesised weight bypasses it. - Tests: TestLoadGemma4...FusedGateUpMatchesSplitExperts now proves the SYNTHESISED fused decode == native gate_up_proj decode (synthesis correctness); the shard-view load tests and the coverage guard (missing-shard-view errors) still hold. Verified by the synthetic parity test + the existing fused-vs-baseline benches (~34%); the 26B-A4B end-to-end tok/s is the final confirmation (RAM-guarded load). Co-Authored-By: Virgil <virgil@lethean.io>
…tok/s The submit-ahead decode (two ICBs over shared KV, host submits token t+1 before reading t; discard-safe for greedy because the chain feeds each step's input on-GPU from the prior argmax) sat behind pipelinedGPUDecodeEnabled=false — "off by default until soaked" — so every serve/generate ran the chained sync-per-token lane. The probe suite has been driving it explicitly for weeks; production never got it. This IS the go-mlx-era knowledge the port carried but never re-enabled. Soak receipts (M3 Ultra, gemma-4-e2b-it-4bit, real checkpoint): - TestRealE2BChainedGPUParityAndSpeed (tg64): host 161.3 / chained 165.8 (gpu-busy 90%) / pipelined 181.8 (gpu-busy 100%) — tokens IDENTICAL. - TestRealE2BDecodeDeterministic green; full metal_runtime suite green -count=1 (126s) WITH the new default, untagged suite green. - End-to-end lem-dev generate (temp0, decode-only): tg192 161.6→178.2, tg448 158.8→173.2. Test hygiene the flip requires: restore sites that hard-reset the flag to false (the old default) now restore true, so a lane-comparison test no longer strands the rest of the suite on the unpipelined path — the oldPipe save/restore sites are untouched. Setup lines that force a specific lane (chained-vs-pipelined comparisons) keep forcing it. Sampled decode and MoE routing are unaffected (their gates never took the chain); eosID-bearing one-shots keep the chained lane per the existing gate. Co-Authored-By: Virgil <virgil@lethean.io>
Pre-existing drift — decode/tokenizer sorted before model and similar import-order skew across engine/metal, engine/hip, kv, model and cmd. The qa gate (test -z "$(gofmt -l .)") was failing on dev independent of any feature work. Mechanical gofmt -w; no code change. Co-Authored-By: Virgil <virgil@lethean.io>
…and dropped lem generate -pipeline (default true) claimed to force the serial loop for A/B traces but never reached the engine; generate.Config.Pipeline was a dead field. engine/metal now exports SetPipelinedGPUDecode (the knob behind the c9434bd default) and cmd/lem applies the flag before RunGenerate — cmd/lem is darwin/arm64-only (main.go blank-imports the tagged engine), so the direct call is sound. Proven end-to-end on e2b-4bit (tg128, decode-only): -pipeline=false 162.1 tok/s (chained serial) vs default 177.8 (pipelined) — the A/B the flag documented now actually happens. TestSetPipelinedGPUDecode pins the setter. Co-Authored-By: Virgil <virgil@lethean.io>
…nd_of_turn>
Two stacked display bugs made every served reply unreadable:
1. Spaces vanished ("//ListNodedefinesthestructure"): engine.TextModel
streamed per-token text through DecodeOne, whose Decode-of-one contract
STRIPS the SentencePiece word-boundary space. Streaming now uses
DecodeToken (boundary space kept, specials hidden, gemma4 channel
markers preserved for the extractor); Classify keeps the stripped form
via decodeLabel — a label token should decode clean.
2. Every reply ended with a literal "<end_of_turn>": gemma4 MLX snapshots
ship the turn terminator as a PLAIN vocab token (absent from
tokenizer.json added_tokens — eos is <eos>), so no decode layer can hide
it and the string-level stop machinery fires AFTER its text lands in
content. Both marker machines now swallow the bare terminator from the
visible lane: serving/provider/openai.ThinkingExtractor (the live
/v1/chat path) and decode/parser.Processor (the state-session lane, via
a per-family terminators set — gemma only). Split-across-tokens arrivals
ride the existing partial-suffix holdback; span-end matching inside a
thinking block is unchanged. Engine stop-token yields also blank their
text (id still surfaces for trackers).
Proven live on e2b-4bit serve: "the answer ... = 40 + 28 = 68$.\n" — real
spaces, no terminator, finish=stop. Regression tests: engine/model_test.go
(first tests in the package), parser terminator quartet, extractor trio.
Full untagged suite + metal_runtime suite green -count=1.
Co-Authored-By: Virgil <virgil@lethean.io>
…ecode — 12B +17%, 31B +15%
The GPU next-inputs seam was e2b-shaped only ("Handles e2b's shape only"):
it required the PLE tower, so 12B/31B — which have no per-layer-input
tower at all — failed the encNextInputsGPU/plScratchNew chain gates and
every request ran the host per-token loop.
A non-PLE quant session now gets the reduced seam: the only per-step
input is the token embedding, so encNextInputsGPU is the embed gather
alone and plScratchNew hands out a zero-value placeholder the closure
never reads (it exists to satisfy the chained/pipelined gates; every
sc.out = icb.pleInput write is a nil onto an unused field — the recorder
already carries hasPLE=false and pli=nil for these archs).
Receipts (M3 Ultra, temp0, decode-only via lem-dev generate):
12B-4bit: host ~62 -> chained 70.7 -> pipelined 73.7 tok/s
(now ahead of llama.cpp ~64 on dense 12B; go-mlx era was ~52)
31B-4bit: ~31 -> pipelined 36.8 tok/s
e2b unchanged (own PLE seam).
Parity: TestDenseGPUDecodeSeamEngages pins the seam wiring (without it a
parity test compares host against host); TestDensePipelinedGPUDecodeMatchesHost
proves host/chained/pipelined token-identical on the dense fixture. Full
metal_runtime suite green -count=1 (the seam now engages for every
non-PLE fixture in the suite), untagged green.
Remaining in this lane: the qat shapes (quantised PLE projection) still
decline the seam — tracked in #286.
Co-Authored-By: Virgil <virgil@lethean.io>
…b-qat +26%, e4b-qat +22% Second half of the fleet extension (after 1afce40 opened the non-PLE lane): the GPU next-inputs seam declined the QAT shape because the per-layer model projection ships QUANTISED (verified from the snapshots: e2b-qat [8960,192] U32 / e4b-qat [10752,320] — both 4-bit gs=64 affine with the geometry read from the shapes), and the seam only knew the bf16-projection e2b shape. qat models therefore ran the host per-token loop — with the host ALSO paying a quant qmv PLE projection per token, which is why qat-e2b sat at ~118 while non-qat did 162. encPerLayerInputsGPUQuantProjObject is the quant-proj sibling of the GPU PLE stage: op 2 (the projection matvec) runs as the standard steel qmv over the packed weight — the same kernel the host PerLayerInputs dispatches — and the other five ops are byte-identical to the bf16 lane. The seam wires it for bits==4 + quantised PLE embedding + quantised projection (own gs/bits > 0). Receipts (M3 Ultra, temp0, decode-only via lem-dev generate, tg256): e2b-qat: ~118 -> pipelined 148.7 tok/s e4b-qat: ~78 -> pipelined 95.5 tok/s Parity: qatPLEQuantModel (quantised-projection fixture) + TestQATGPUDecodeSeamEngages + TestQATPipelinedGPUDecodeMatchesHost prove host/chained/pipelined token-identical on the qat shape. Full metal_runtime suite green -count=1, untagged green. With 1afce40 this closes the fleet gap: every dense gemma4 quant shape (non-qat E-family, qat E-family, 12B, 31B) now rides the submit-ahead decode. MoE (26B-A4B) remains on its own path. Co-Authored-By: Virgil <virgil@lethean.io>
…e 45 -> 131 tok/s The serve-realistic sampled params (temp 0.8, top_k 40, top_p 0.95) ran at 45 tok/s while greedy did 144 — 22ms/token, and profiling put it all in ONE place: every GPU top-k SELECTION kernel is value-dependent and slow on real logits at real vocab (e2b 262k, M3 Ultra): fused Q4 tiles 22.6ms, qmv+topk tiles 17.7ms, logits-sample@k 33ms — while the argmax tiles run 0.58ms and a full-logits readback costs 0.56ms. (Synthetic-hidden microbenches flattered the kernels 7-10x; only real logits expose the selection churn — the probe harness that caught this is committed as sampled_lane_probe_test.go.) A one-pass host K-array select (K <= 64, tracked minimum, original bf16 bit patterns, zero alloc — 0.26ms at 262k) now feeds model.SampleCandidates, and the TopK>1 eligibility gates (topK-token, topK-candidates, logits-token, retained-logits) decline at real vocab sizes so the ladder falls through to logits + host select. Fixture-sized vocabs (< 4096) stay on the GPU lanes — their chained-sampled parity tests exercise those paths unchanged. Mathematically identical sampling: softmax ratios survive subsetting, so candidates-then-sample equals full-vocab softmax restricted to its top-k. Receipts (real e2b-4bit): session GenerateSampledEach temp0.8+topk40: 28.55 -> 7.38 ms/token (35 -> 135 tok/s) BenchmarkRealE2BContractSampled (the 45.5 headline): 21.9 -> 7.65 ms/token (45.5 -> 130.7 tok/s) greedy + temp-only + topp-only lanes unchanged. hostTopKCandidatesBF16: 262950 ns/op, 0 B/op, 0 allocs/op. Selector pinned against a sort-based reference (set-equality by value at the tie boundary, original bf16 bytes, suppression, k>vocab clamp, seeded determinism). Full metal_runtime suite green -count=1, untagged green. Co-Authored-By: Virgil <virgil@lethean.io>
…ractor + tokenizer consume it Three copies of the marker grammar lived side by side: decode/parser (the state-session Processor, per-family registry), serving/provider/openai (ThinkingExtractor, the live /v1/chat lane) and decode/tokenizer (the channel consts DecodeToken preserves). Every grammar fix landed N times — the <end_of_turn> terminator in 3825bc8 had to be written twice, which is what filed this task. decode/parser/grammar.go is now the single source: ChannelOpenMarker / ChannelCloseMarker / GemmaTurnTerminator consts, the PairedMarker table (PairedReasoningMarkers) and IsReasoningChannel. The Processor derives genericMarkers from the table and builds the gemma/gpt-oss channel spans from the consts; the extractor aliases the consts, ranges the shared table and classifies channels via IsReasoningChannel (its zero-alloc hot paths untouched); the tokenizer consts alias the parser ones. Two deliberate unifications the extractor gains from the richer table: <analysis>...</analysis> is now a recognised paired span, and the "analysis" channel routes to thought — gpt-oss harmony treats analysis AS its reasoning channel; the extractor previously leaked it into content. No test pinned the old behaviour. Grammar consistency is itself pinned: TestPairedReasoningMarkers_Bad fails if the Processor's derived set drifts from the authoritative table. Full decode/serving/model suites green (61 pkgs, -count=1); live serve smoke clean (spaces, no terminator, finish=stop). Co-Authored-By: Virgil <virgil@lethean.io>
…ams (#287)
The three flags printed "seam not yet exposed" notes. Wire them to real,
honest engine seams instead — no fabricated support.
-trace — the perf-work enabler. New engine-neutral inference.DecodePhaseBudget
(aggregate per-token GPU-busy vs host-serial split + engine-named phases) on
GenerateMetrics.DecodePhases. The metal ArchSession implements the optional
engine.DecodePhaseTracer, flipping the pieceTiming counters through the
PRODUCTION decode path and folding the totals into the budget; engine.TextModel
begins the trace when GenerateConfig.TraceTokenPhases is set. RunGenerate prints
the budget table (GPU-busy %, host-serial % + tok/s ceiling, per-phase shares)
— the "where do the per-token ms go" answer that previously needed test-only
globals. This is THIS engine's real measurement (3 GPU pieces + chained GPU
span), not a port of go-mlx's 20-field lazy-eval trace.
live e2b-4bit greedy: 18 tok · 5.564 ms/token · 179.7 tok/s
GPU busy 98.4% · host serial 1.6% (ceiling if zeroed: 182.6)
-kv-cache / -kv-storage — capability seam. engine.TextModel.Capabilities()
forwards the concrete engine's SupportedCacheModes (engine.CacheModeReporter)
onto the existing inference.CapabilityReport.CacheModes. The metal engine
declares its one native cache honestly (no go-mlx-era fp16/q8/kq8vq4/turboquant
selector ever ported). noteCacheKnobs consults the report: an override the
engine doesn't honour prints an accurate "ignored, this engine supports: native"
note; a declared mode is silent; future engines that honour a selector list it
through the same seam.
Tests: DecodePhaseBudget host/GPU split + clamps; buildDecodePhaseBudget for the
chained-span and 3-piece paths + zero-token guard; BeginDecodePhaseTrace pos-
delta + nil-safe; SupportedCacheModes; Capabilities cache-mode forwarding +
absence; setDecodePhases; noteCacheKnobs honoured/unhonoured/storage;
cacheModeHonoured; printDecodePhaseBudget; tokPerSec. Full decode/engine/serving
suites green; live generate -trace / -kv-cache verified.
Co-Authored-By: Virgil <virgil@lethean.io>
… on -state sleeps (#289)
Follow-up to #287. The live -kv-cache is single-mode (one paged bf16 cache; no
live turboquant encoder exists here) — its capability note stays. The genuine
selectable KV-state knob is the portable snapshot kv.Encoding, threaded through
the -state sleep path (agent.SleepOptions.BlockOptions.KVEncoding).
-kv-storage now resolves to kv.Encoding and is honestly classified:
- native → threaded into the sleep; live-verified (stores + sleeps).
- q8/float32 → recognised but NOT yet produce-able from a live metal -state
sleep: the block capture (nativeKVLayerBlockSnapshot) emits the
layer-level raw bf16 tensor, so the q8/float32 snapshot encoder
gets a raw tensor with no layer float32 and rejects it
(errRawTensorNeedsNative). They fall back to native with an
honest note rather than hard-erroring the sleep — the encode-
from-per-head-float32 codec fix is a scoped follow-up.
- unknown → noted (native/q8/float32 are the set) + native fallback.
- stateless → inert note (a bench run persists no KV).
Flag help corrected from the go-mlx-era "fp16, bf16" to the real encoding set.
Engine-neutral: the encodings live in the kv package, validated directly rather
than through the engine capability seam (#287 covers the engine-specific live
cache mode).
Tests: kvStorageEncoding 3-way classification (native/q8/float32/unknown);
noteKVStorageInert good/bad/ugly; noteCacheKnobs now -kv-cache-only. generate +
model/state suites green; live native/q8/stateless paths verified.
Co-Authored-By: Virgil <virgil@lethean.io>
…e sleep (#290)
Unblocks -kv-storage q8/float32 (#289 fell them back to native). The metal block
capture (nativeKVLayerBlockSnapshot) always emitted the layer-level raw bf16
tensor; even at RawKVOnly=false it added per-head float32 in layer.Heads but KEPT
the raw layer slab, so the snapshot encoder was handed a raw tensor with no
layer-level float32 and rejected non-native encodings with errRawTensorNeedsNative
("KV snapshot raw tensor requires native encoding").
Fix: the non-native (RawKVOnly=false) block capture now emits ONLY the per-head
float32 tensors — the layer-level raw bf16 slab could only encode as native, and
dropping the redundant copy also makes the q8 store smaller. native's fast path
(RawKVOnly=true) is unchanged; restore already falls back to
nativeKVHeadSnapshotSlabs when the layer-level slab is absent. The non-native
block path was previously dead (only q8/float32 reached it, and they errored), so
no live path regresses.
kvStorageEncoding now reports q8/float32 as usable; the recognised/liveStateReady
split collapses to recognised (all three real encodings work).
Proof — TestSessionStateBlockNonNativeEncodingRoundTrip: capture RawKVOnly=false →
Save → Load → RestoreFromKV → GenerateFromCache. float32 is LOSSLESS →
token-identical to the uninterrupted run; q8 round-trips WITHOUT error and
continues with in-range tokens. Live: `generate -state -kv-storage q8` sleeps
("stores KV as q8"), store smaller than native. Full kv/state/generate/metal
suites green.
Note: the generate -state WAKE glue is a separate pre-existing gap — native wake
also fails "native model session does not support KV restore" (the CLI wake path,
not the codec). Filed separately.
Co-Authored-By: Virgil <virgil@lethean.io>
…ine gates, opt-in lane (#351) UnifiedVisionImagePatches rides the shared Gemma 4 sizing rule (VisionImagePixels) and emits the reference's exact byte order, verbatim- matched to the HF processor: teacher patches flatten [py][px][c] (the permute(1,3,2,4,0) row reshape), model patches concatenate their 3x3 kernel row-major, and position ids are (x, y) = (COL, ROW) — the pos table's axis 0 is the column (meshgrid indexing="xy"). ProjectImage, AcceptsImageInput and the placeholder accessors branch on the unified payload; per-image soft-token counts (no pad tokens) match the processor. End-to-end conviction on the 12B: a red image answers "Red" at forced spans of 1, 4 and 16 soft tokens (LTHN_VISION_MAXTOK instrument), proving preprocess, embedder, splice and prompt convention correct — but the full 256-token span answers "Black": the arch declares use_bidirectional_attention="vision" and a causally-evaluated span of that size drifts out of distribution. Until the span mask lands the unified lane is OPT-IN (LTHN_UNIFIED_VISION=1); the default keeps the clean image-input rejection — wrong answers are worse than a refusal. LTHN_VISION_DIAG prints per-stage magnitude stats. Co-Authored-By: Virgil <virgil@lethean.io>
… by default (#351)
The 12B declares use_bidirectional_attention="vision": image soft-token
spans attend bidirectionally in prefill, and causal evaluation measurably
degrades — the A/B on a red circle reads "a red shape" causal vs "a red
circle" bidirectional; spatial answers ("top half") need the mask too.
The session detects placeholder runs itself (bidirSpanToken, set at
OpenSession from the unified payload's declared bidirectionality — parsed
from the text config). Span prefills route batched-or-error: chunks are
cut so no span straddles one, and each chunk runs the batched dense pass
with per-row attention caps (span rows see through to their span end —
legal because the batched-rope fold lands the WHOLE chunk's K/V before
any SDPA reads). The multiQ/GEMM-SDPA/deferred-ring lanes compute causal
caps in-kernel, so caps force the per-row SDPA and the pass hard-errors
on any lane that would silently evaluate a span causally (per-row K/V
landings, staged rings, prompts past the sliding window). There is
deliberately NO sequential fallback for span prefills.
With the mask receipted (shape + colour + spatial correct on the 12B,
text lanes unregressed at 73.5 tok/s) the unified vision lane now serves
by DEFAULT; LTHN_UNIFIED_VISION=0 restores the image rejection and
LTHN_VISION_BIDIR=0 is the causal A/B lever. Degenerate full-frame
solid-colour images may still misread — a model-side behaviour, not a
pipeline defect (the pipeline is byte-verified through span 16).
Co-Authored-By: Virgil <virgil@lethean.io>
…-end, speech transcribed (#351) The unified audio head is radically simple: each audio token is 640 raw 16 kHz samples (40ms) — no mel front-end, no Conformer — projected straight into the backbone (scale-free RMSNorm → embed_audio), zero-padded to whole tokens, spliced at the audio placeholders AFTER the turn text (the gemma4 audio-after-text convention). The processor's mel fields are vestigial for unified packs; the HF extractor is a reshape. New end-to-end transport (audio had none): inference.Message.Audios + the neutral AudioModel probe, the engine's AudioInputTokenModel capability + chatMultimodal audio branch with its own placeholder-count check, resolveAudioInputs + requireAudio on the generate path, and the -audio flag goes live (WAV 16-bit PCM mono 16 kHz, or a base64 data: URL; anything else errors loud — no silent resample). metal: the unified assembler picks up embed_audio + audio_samples_per_token, DecodeWAVMono16k, UnifiedAudioProject, and the token-model audio gates route unified packs ahead of the Conformer lane. Receipt: `say`-synthesised speech through `lem generate -audio` on the 12B transcribes word-for-word — "The quick brown fox jumps over the lazy dog." Pure synthetic tones read as unintelligible (a degenerate input, as vision's solid-colour frames were); text lanes unregressed; suites green. Co-Authored-By: Virgil <virgil@lethean.io>
…/chat/completions (#351)
The chat-completions route now parses the OpenAI input_audio part
({"type":"input_audio","input_audio":{"data":"<base64>","format":"wav"}})
into inference.Message.Audios, mirroring the image_url lane: bare base64
(bounded before decode, same 32 MiB cap), WAV only (16-bit PCM mono
16 kHz — anything else is a 400, no silent resample), at most 16 parts
per request, and the handler gates audio-bearing requests on the
model's neutral AudioModel capability exactly as images gate on
VisionModel.
Receipt: the speech WAV that lem generate transcribes locally comes back
word-for-word over curl against a served 12B — "The quick brown fox
jumps over the lazy dog."
Co-Authored-By: Virgil <virgil@lethean.io>
…c resample to 16 kHz (#351)
DecodeWAVMono16k now takes 16-bit PCM at any sample rate and up to 8
channels: stereo downmixes by averaging, non-16 kHz resamples through a
32-tap-per-side Kaiser-windowed sinc with the cutoff at the lower
Nyquist (anti-aliased downsampling; wsum-normalised taps keep unity
passband gain). The most common papercut — a 44.1/48 kHz recording —
now just works; non-PCM formats still error loud.
Receipt: 44.1 kHz `say` speech transcribes correctly on the 12B
("She sells seashells by the seashore.").
Co-Authored-By: Virgil <virgil@lethean.io>
…cks through the vision embedder (#353)
Video rides the machinery vision already proved: each sampled frame
patchifies and projects through the SAME unified embedder, splices at
the video placeholder, and renders as one timestamped block per frame —
the reference's "MM:SS {boi}{video_token×N}{eoi}" join (frames treated
as 1 s apart on the frame-input path). The bidirectional span machinery
extends from one placeholder id to two: image AND video runs (the
reference's type-1|2 vision blocks) detected per-token so adjacent
blocks stay separate spans.
Transport end-to-end: inference.Message.Videos (the frames of one video,
in time order), the VideoTokenModel capability + chatMultimodal video
branch with its own placeholder-count check, unified-payload video token
wiring, and the repeatable -video-frame flag (PNG/JPEG path or data:
URL; video rejects cleanly on non-vision models and on -state turns).
Receipts on the 12B: a 3-frame clip of a circle at 20%/50%/80% width
answers "moving right", and the SAME frames reversed answer "moving
left" — motion read from temporal order, not a prior. Text/image/audio
lanes unregressed; suites green.
Co-Authored-By: Virgil <virgil@lethean.io>
… bounded plain stretch + probe (#299) The greedy speculative loop's patience bail no longer retires the drafter for the whole request. It now runs plain for a bounded cooldown (32 tokens, timed live), then re-probes drafting and stays engaged only while the DELIVERED token rate holds at/above the measured plain rate — the banked lesson applied: gate on net rate, never acceptance %. Policy pieces, each earned by a receipt this session: - probe: abort on a fully-rejected cycle (unambiguous, no clock); no engage verdict on cycle 1 (post-plain proposals read systematically high — the burst bias); engage needs plainRate x1.08 (hysteresis — an e2b probe at 2-3 accepted rides exactly on the threshold otherwise). - engaged stretch: 3-cycle rolling rate window exits the moment it drops below plain (the acceptance streak misses 2-of-5/3-of-5 alternation that loses every cycle). - failed probes AND rate-exits double the cooldown (cap 256); a passing probe resets it; a fresh patience bail restarts at the minimum. Receipts (12B-4bit + bf16 assistant, block 5, temp 0): - primes-100: 100.5 tok/s / 87% acceptance — unchanged (never bails). - code prompt: 71.9-72.1 vs permanent-bail 72.7 (within noise; was 69.3 before the anti-flap pieces). - e2b guard (net-negative pair, plain 174.5): all probes correctly refused (60-113 vs bar ~187), ladder 32-64-128; 159.1 vs permanent-bail 165.5 — the exploration tax, amortises past 512 tokens. - LTHN_MTP_REENGAGE=0 restores the permanent bail BYTE-EXACTLY (diffed against the 8651b6a binary on two prompts, text + mtp stats identical). Also banked: the parked ICB-recorded-verify lever is dead — LTHN_GPU_TRACE shows the K=5 verify forward GPU-bound (gpuTotal 27.3-27.6ms vs 27.2ms wall; mlp 67%), so there is no encode tax to record away. Pair-vs-plain token divergence at quant near-ties predates this change (8651b6a pair already forks from plain at "199" on primes) — verify-fold numerics, not scheduling. The sampled lane (serve temp>0) keeps the permanent bail pending its own receipts. Co-Authored-By: Virgil <virgil@lethean.io>
…o mtpReengage (#299) The #299 rate-gated re-engagement policy moves out of the greedy loop's closures into one shared struct (mtp_reengage.go) — probe verdicts, the rolling engaged-rate window, and the cooldown ladder in a single place — and the sampled lane (what serve runs at temp>0) now uses it too via a bounded sampled plain-run (nativeAssistantPlainRunSampledFromTargetCache, history threaded through the stretch). Both lanes keep the permanent bail under LTHN_MTP_REENGAGE=0. Receipts (12B-4bit + bf16 assistant, block 5): - greedy unchanged through the refactor: code 72.2 tok/s / 12 verifies, primes 100.7 / 87%; kill-switch output byte-exact vs the 78b42db binary. - sampled temp 0.7: kill-switch byte-exact vs the 78b42db binary (same seed); re-engagement live (bail@85, probe fail 45.1, engage 79.0 vs bar ~77, rate-exit@221, doubling ladder), rates within the same +/-2 envelope as greedy (69.6 on vs 70.8 off vs 71.7 plain). - policy unit tests host-side (no GPU): bail ladder, probe abort / cycle-1 no-engage / cycle-2 engage / mediocre full-ride, window inert-until-plain / exit-below / hold-above. Engine/metal suite + whole repo green. Co-Authored-By: Virgil <virgil@lethean.io>
…ir prefill 184s -> 16.6s (#299) prefillPromptCacheEntry (prepareAssistantPrompt + WarmPromptCache) split the prompt into prefillCachedIDs + one retained step; on recorded-ICB sessions prefillCachedIDs routed the WHOLE prompt down the chained per-token GPU-inputs lane (~11ms/token on the 12B). A 16.6K-token prompt with a drafter armed prefilled in 184s while the plain path took 18.5s — found by the #299 depth sweep. It now rides the same prefillRetainedTokens lane as plain generate (chunked batched dense + GPU PLE slab, per-token only as that lane's own fallback). Receipts (12B-4bit + bf16 assistant, 16.6K-token prompt): - pair prefill 184.1s -> 16.6s (11x); total 1.1 -> 9.7 tok/s - output IDENTICAL pre/post fix (same tokens, same 80% acceptance, same 47 verify forwards) - short-context pair unchanged: primes 100.0 tok/s / 87% - engine/metal suite + whole repo green Co-Authored-By: Virgil <virgil@lethean.io>
…0ms, pair 78.2 -> 97.6 tok/s (#354) The MTP verify on MoE targets ran K sequential full steps: the batchMoE admission required K > batchedDenseICBMaxRows, so the K-row MoE block never engaged at verify widths (26B K=5 verify = 35.7ms against a ~7ms plain step; the pair HALVED throughput at 81% acceptance). Three measured tiers landed: - admission: batchMoE also admits verifyFoldSmallK at K>1 (the fold slabs exist there; the naive flip alone measured WORSE — 65.5 tok/s, moe stage 34.1ms of 38.6 by LTHN_GPU_TRACE — the small-M qmm occupancy collapse). - expert lane: the verify's 16-48 pairs route to the all-routes gathers (84.2 vs 65.5 tok/s measured) — grouped-GEMM's sort/gather/scatter machinery only amortises at prompt-scale pair counts. - router + local MLP: encQMVRowsBF16At under the verify tier (the same multi-row-qmv swap that took the dense verify 66 -> 27ms), qmm_t otherwise. All three scope on s.verifyFoldSmallK: prompt-scale prefill bytes are untouched (26B 16.6K prefill 7.85s, unchanged). Verify acceptance identical (81%, 187/231, 47 forwards). 12B pair unchanged (100.6 / 87%). Suites green. Remaining pair gap to plain (97.6 vs 143.7) is the drafter's kvExport — 10.8-14.2ms per block on the 26B even at short context (#355). Co-Authored-By: Virgil <virgil@lethean.io>
…r 48.5 -> 71.5 (beats plain), 26B pair 97.6 -> 134.1 (#355) The per-block target-KV export materialised EVERY owner cache (12B: 48, 26B: 30) while the drafter consumes only the LAST target layer of each layer type — targetKVByLayerType's set() overwrites per type, so all but ~2 streams were transposed and thrown away. Cost: 47-72ms per draft block on the 12B at 16K context; 10-14ms per block on the 26B even short. Two cuts, same winner-resolution helper (assistantKVWinnerCaches): - the session exporter transposes only winner caches; targetKVByLayerType filters to winners (identical output — non-winners were overwritten). - stateLayerViewsRefreshing(needed): the paged-KV snapshot re-copy (the 26B's fixed ~10ms/block: refreshPagedStateLayerViews walked all 30 layers) refreshes only the named caches on the export path. All state save/restore paths keep the unfiltered entry, and every reload site calls the unfiltered refresh itself first, so no stale bytes can reach a page reload. Receipts (temp 0, block 5, acceptance identical everywhere): - 12B @16k: kvExport 47-72ms -> 1.2-1.8ms/block; pair decode 48.5 -> 71.5 tok/s — the pair now BEATS plain (62.8) at depth (+14%). - 26B: kvExport steady 13.4 -> 0.5ms/block; pair 97.6 -> 134.1 tok/s (78.2 this morning; plain 143.7). Block sweep: 5/6 tie, 8 decays. - 12B short: 100.5 -> 107.5 tok/s (it paid the 48-cache walk too). - e2b guard unchanged (155.5, rate gate still refuses a net-negative pair). - engine/metal suite + whole repo green. Co-Authored-By: Virgil <virgil@lethean.io>
…eep scans, byte-identical (#356) lthn_sdpa_paged_p1_gqa2_bf16 (+ _final_ variant): one threadgroup per (KV head, split window) streams the window's rows ONCE and carries both query heads' online-softmax states — the per-head kernel read every cached row once per QUERY head, so GQA-2 models (26B/31B/12B) paid 2x the bandwidth floor on the depth-scaling loop. Row order, accumulation order and the simdgroup merge are identical per head: 26B outputs at temp 0 are byte-identical to the per-head kernel, short and 16K. Occupancy lessons, both measured the hard way: - the naive port ran SLOWER (4.15 vs 3.82 ms/token attn.sdpa) — dropping the query-head grid factor left 64 threadgroups on an 80-core GPU, and the compile-sized [2][16] accumulator arrays doubled the register cost. The grain halves (splitRows 256 -> 128) to restore the threadgroup count and the gqa2 arrays size for headDim<=256 (every gqa2 model's shape; host-gated + in-kernel belt). - short windows keep the per-head kernel: halving an already-tiny grid measured 143.7 -> 138.1 tok/s on the 26B short decode; the gqa2 route engages only past one split window of rows. Receipts: 26B short 143.2 (unregressed), 26B @16k 104.7 (par: the deep scan is LATENCY-bound, not bandwidth-bound — grain sweep 64/128/512 gave 5.4/4.4/6.0 ms/token, untracked-scratch probe was null — so the halved traffic banks as headroom, not tok/s; the latency chain is #356's next unit, LTHN_SDPA_SPLIT probe lever included). Paged parity suite + whole repo green. Also: -kv-cache help text stops advertising the go-mlx-era modes the no-cgo engine never ported (the runtime already noted+ignored them). Co-Authored-By: Virgil <virgil@lethean.io>
…16k 103.0 -> 106.6 TestSDPAPagedDepthAnatomy (LEM_SDPA_ANATOMY=1) is the instrument the live profiler could not be: it dispatches JUST the paged P1/P2 pair at the 26B deep-scan shape and reads the GPU span directly. The map it produced: - one global-layer scan costs 0.487ms at production shape (the live 3.8ms attn.sdpa is 5 globals on the critical path plus sliding); - the same rows in ONE page run 26% faster (0.367) — dispatch boundaries, not hazards, are the biggest remaining lever (page sizing, host-side); - the per-head kernel reaches 637GB/s at one page (78% of bandwidth); the gqa2 kernel's dual chains sit at ~365 — per-ROW work, not bytes, prices the scan (which is why halving traffic was tok/s-par in fe76c56); - split-grain 64/128/256 = 0.775/0.487/0.567: 128 stands. The unroll: the gqa2 row loop takes rows (t, t+8) per iteration — four independent K-dots/simd_sums pipeline, one BATCHED online-softmax fold (s·e^(m−M) + e^(d1−M) + e^(d2−M), the two sequential updates composed). A numeric tier (exp identities round differently), the verify-fold precedent — measured receipts: bench 0.514 -> 0.487, live 26B @16k 104.7 -> 106.6, short 143.2 unregressed, 16K output IDENTICAL to the pre-unroll kernel on this run, primes correct, paged parity + whole repo green. LTHN_SDPA_SPLIT stays as the grain probe. Next units banked in #356/#357: page sizing (the -26% dispatch lever), live quantised KV (multiplies with the gqa2 traffic halving). Co-Authored-By: Virgil <virgil@lethean.io>
… logarithmic (#356) Device paged-KV pages now double from the base (2048 -> 4096 -> 8192 -> 16384, then flat): the paged SDPA runs one pass-1 dispatch per visited page, and the #356 anatomy bench measured the 16K scan 26% faster in one page than in eight. Fixed-size division becomes a closed-form schedule (pageRowsFor / pageStartFor / pageForPos — bit-length over the doubling ramp, flat steps past the cap); slot, prealloc, truncate and both linear snapshot directions share it. Page layout and per-page strides were already size-independent, so the SDPA plan and kernels are untouched. Dispatch count per global layer: 16K context 8 -> 4, 32K 16 -> 5, 128K 63 -> 11. Allocation granularity stays 2048 rows for short sessions; worst-case over-allocation is one 16K page. Receipts: 26B @16k 106.6 -> 107.2 (the live critical path feels dispatch boundaries far less than the bench's overlapped stream — the win is the depth SCALING, not a 16K step); 26B @32k 89.1 tok/s (new baseline; the depth curve is now 143/107/89 at 0/16K/32K); short 142.6 unregressed; 16K output IDENTICAL; geometry schedule + round-trip unit tests; the two uniform-schedule pinning tests updated; engine/metal + whole repo green. Co-Authored-By: Virgil <virgil@lethean.io>
… 26B @16k 107.2 -> 110.7 (#356) Stage-elision probes (identical K/V traffic, stages removed) priced the gqa2 row loop at the 1x16K shape: loads+dots 0.161ms/scan (832 GB/s — the memory system was never the problem), +simd_sums 0.168, the COMPLETE row loop with exp state folds 0.178. The full kernel measured 0.367: half the kernel was the threadgroup merge — 7 of 8 simdgroups parked at two barriers while simdgroup 0 folded all 256 dims serially, twice (the two GQA phases). The merge now runs parallel over head dims: every thread (global id, not lane) owns headDim/256 dims, folds the 8 simdgroup partials for its dims, and writes its cells/final rows directly — coalesced. The 8 scalar M/S folds are recomputed per thread: redundant flops instead of an extra barrier. Probe kernels removed after the verdict; findings live here. Receipts: anatomy scan 0.487 -> 0.394 (8 pages), 0.367 -> 0.338 (1 page); live 26B @16k 107.2 -> 110.7, @32k 89.1 -> 91.6, short 143.5 unregressed; 16K output IDENTICAL; paged parity + engine/metal + whole repo green. Depth curve for the day: 16K 103.0 -> 110.7, 32K (new) 91.6. Remaining #356 headroom: anatomy P1-only floor is 0.178/scan vs 0.338 with P2+drain — the inter-pass drain and the under-occupied P2 merge are the next slice. Co-Authored-By: Virgil <virgil@lethean.io>
…@32k 91.6 -> 98.3 (#356) Pass 2 merged cells at one 32-lane simdgroup per head — 512 threads machine-wide folding cellCount cells over 8 dims per lane serially, the same under-parallelised shape the P1 merge fix addressed. It now runs 256 threads per head, one thread per head dim, each folding the cells for its dim with the per-thread scalar M/S refold (redundant flops, no reduction, coalesced writes). Receipts: anatomy scan 0.394 -> 0.312 ms (8 pages), 0.338 -> 0.256 (1 page) — the day's kernel work has taken the production scan 0.514 -> 0.312 (-39%); live 26B @16k 110.7 -> 116.2, @32k 91.6 -> 98.3, short 144.2 unregressed; 16K output IDENTICAL; paged parity + engine/metal + whole repo green. Depth curve day summary: 16K 103.0 -> 116.2 (+12.8%), 32K 89.1 -> 98.3 (+10.3%); the short-vs-16K drop narrowed -28% -> -19%. Remaining scan headroom vs the P1-only floor (0.256 vs 0.178) is the inter-pass drain; #357's quant KV halves the bytes under all of it. Co-Authored-By: Virgil <virgil@lethean.io>
…t depth instead of riding net-negative (#358) The #299 rate gate armed only on the first acceptance-streak bail, so a drafter that stays strong never measured a plain rate — and deep-context verify costs (SDPA + KV sync scale with position; drafting does not) left the 26B pair fully engaged at HALF plain (58.4 vs 116.2 tok/s at 74% acceptance, 16K context). Three pieces, each forced by a receipt: - deep bootstrap: past 8192 attended tokens with no measured plain rate (and budget to pay it back), the loop spends ONE 8-token plain stretch to arm the gate. Shallow runs never trigger it. - probe cycle 1 is pure warmup — no verdict AND no clock, both directions: shallow it reads high (burst bias), deep it reads LOW (the drafter re-uploads its target-KV mirror after any plain stretch, ~40ms at 16K; probes measured 26.6 tok/s where the steady pair runs ~71). - no abort on a fully-rejected WARMUP block either — the first block after a re-seed can fully miss even when the steady pair wins; the abort now applies only to measured cycles. Receipts (16K context, temp 0, block 5): - 26B pair: 58.4 -> 92.4 tok/s (+58%; bootstrap fires at pos ~13.8K, probes correctly refuse, 7 verifies vs 49) - 12B pair: 72.1 tok/s, 44 verifies, 84% acceptance — the deep WIN case keeps full engagement (71.5 before the change; probe tax ~nil) - short pairs untouched (26B 133.0, pos < threshold never triggers) - policy unit tests extended (bootstrap predicate, warmup semantics); the engage-from-cycle-two test backdates the warmup-reset clock (two calls can land in one clock tick); engine/metal + whole repo green. Co-Authored-By: Virgil <virgil@lethean.io>
…uant/dequant, gqa2 read kernels, GPU parity (#357) Stage 1+2 of the live quantised KV cache. Pages hold int8 rows quantised symmetrically per 64-element group with f32 scales in PARALLEL pages — same [row][kvHead][dim] order and ELEMENT strides as bf16 pages, so the SDPA addressing carries over with a byte element and one scale load per lane slice (a lane's per=8 slice never straddles a group at headDim 256). The linear twin stays bf16: loadLinearSnapshot quantises on the way in, linearSnapshot dequantises on the way out — prefill, the batched verify, state save/restore and the drafter export all flow through UNCHANGED. Geometric page growth carries over; scale pages grow in lockstep (appendPage), and the worst host round-trip error sits exactly at the group bound (0.0625 over 768K elements, unit-tested). Read side: lthn_sdpa_paged_p1_gqa2_q8_bf16 (+ _final_ single-cell variant) — the scale multiplies each lane's partial BEFORE simd_sum and the p.v product after; 2-row unroll and the dim-parallel merge match the bf16 gqa2 kernel. lthn_kv_q8_store_bf16 (the landing hop) ships with this commit; wiring lands next. attachQ8 swaps a built plan onto the q8 pipelines and FORCES the gqa2 route (there is no per-head q8 kernel) — a q8 cache with a missing kernel is a hard error, never a bf16 fallback misreading int8 pages. GPU parity: q8 vs bf16 plans over identical (dequantised) content — multi-cell 5000 rows and single-cell 100 rows, worst |d| within bf16 rounding. engine/metal suite green (1451). Remaining in #357: the landing rewire (project->scratch->q8-store, both attention passes), cache-mode plumbing (-kv-cache q8 on gqa2 geometry), and the end-to-end 26B receipts. Co-Authored-By: Virgil <virgil@lethean.io>
…LTHN_KV_Q8 lever; 26B par tok/s at HALF the KV bytes (#357) Stage 3 completes the live q8 cache. The projections cannot write int8 pages, so q8 landings project K into sc.kProj and V into the new vProj scratch, run the rope/norms there, and a quantise-store hop (encKVQ8Store -> lthn_kv_q8_store_bf16) writes the page row + group scales — wired through BOTH attention passes (the concurrent 7-stage pass gains a stage-3.5 barrier + stores; the serial fallback rides hazard tracking) and the read plans attach the q8 pipelines (encAttnHalfKVPaged + the shared read-only variant). Mode: LTHN_KV_Q8=1, applied PER LAYER where geometry fits the q8 kernels (gqa2: nHeads == 2*kvHeads, headDim <= 256) — mixed q8/bf16 caches are by design (every landing and read site branches per cache); if NO layer qualifies the init fails loudly rather than silently downgrading. The -kv-cache selector graduates after soak. The host quant on prefill reload was the surprise cost: single-threaded scalar Go took a 16K 26B prefill from 7.9s to 17.6s. parallelRows fans the row loop across cores: 9.7s (+1.8s honest quant work; a GPU-side reload via the store kernel over the linear buffer is the banked eraser). Receipts (26B-A4B, temp 0): primes correct at short AND 16K depth; decode 144.4 short / 115.3 @16k / 96.8 @32k — par with bf16 (144.2 / 116.2 / 98.3) exactly as banked: the scan is latency-bound post-#356, so the halved bytes land as KV MEMORY (~2x on q8 layers) + bandwidth headroom, not tok/s yet. engine/metal 1451 + whole repo 10514 green. Co-Authored-By: Virgil <virgil@lethean.io>
LTHN_KV_Q8, LTHN_MTP_REENGAGE, LTHN_SDPA_SPLIT, LTHN_MTP_DIAG — the load-bearing set from the deep-decode campaign, flagged non-exhaustive (19 levers exist in-tree). Co-Authored-By: Virgil <virgil@lethean.io>
…only) Comment-column alignment, operator spacing, one blank line — no semantic change; suite green (10514/118 pkgs) immediately prior, build+vet re-verified after. Co-Authored-By: Virgil <virgil@lethean.io>
…bration force mode (#359) Per drafted token, the drafter's softmax probability of its own greedy pick (host pass over the already-CPU-visible logits row, suppression mirrored) rides AssistantDraftBlockResult.Probs; the greedy generate loop appends one JSONL cycle line pairing probs with the verified accepted-prefix length and carry offset. LTHN_MTP_CONF_FORCE=1 bypasses the low-accept bail, deep bootstrap, and rate-exit verdicts so calibration samples the full regime distribution (the gate's economics select against exactly the cycles the curve needs); both levers off = byte-identical behaviour, zero extra work. Receipts: 26B 370 cycles/1836 drafts + 12B 250/1245 — reliability monotone (top bin 91%/89% match), cumprod truncation at theta=0.40 retains 99.2%/99.1% of accepted tokens for 8-9% draft work saved on both drafters; 40%/30% of cycles accept the full K=4 with median end-of-block cumprod 0.995/0.979 (the cap raise is the bigger lever, especially at depth: 83% acceptance at 16K). Co-Authored-By: Virgil <virgil@lethean.io>
…acceptance streaks (#359); 26B pair @16k 92.0 -> 99.8 tok/s The calibration curves' loudest finding converted: 40% of 26B cycles accept the whole fixed K=4 block (median end-of-block cumprod 0.995), densest exactly where verify forwards are dearest (83% acceptance at 16K depth). mtpDraftLen raises the cap 4 -> 6 -> 8 after consecutive fully-accepted cycles and resets to base on any partial cycle; raises are gated to pos >= 4096 because the live A/B priced the ungated version (+8.6% at 16K but -2.7% shallow where blocks mostly miss at 48% acceptance) — below the gate the loop is byte-identical to the fixed-K engine, pinned by test. Output is unchanged by construction (verify pins the emitted stream to the target's greedy continuation at any block length). Zero per-token cost: acceptance history only — the cumprod theta-stop (0.40, measured) stays banked in #359 for the live-prob slice. Receipts: 26B @16k 92.0 -> 99.9 ungated / 99.8 gated (twice, identical output), verify forwards 10 -> 9; shallow 123.7 preserved by the gate; kill switch LTHN_MTP_DRAFTLEN=0; suite 1942/17 pkgs green. Co-Authored-By: Virgil <virgil@lethean.io>
…the next driver CLAUDE.md was April-vintage (pure interface package, stdlib-only) — now reflects reality: module root go/, the no-cgo metal engine, cmd/lem, the build/test/kernel flows, the receipts discipline, and current perf state. docs/handover.md carries what code can't: the method (instrument first, no receipt no claim/keep, kill-switch adaptive behaviour), the priced traps (test caching, bench->live transfer limits, gate anti-selection, carry off-by-one, stale worktrees), the open #359 theta-stop slice, and a reading order. Co-Authored-By: Virgil <virgil@lethean.io>
|
Snider
added a commit
that referenced
this pull request
Jul 14, 2026
…nders tasked Audited each design-*.md against the tree (symbols, wiring, receipts), not against its own status lines — two of them undersold what had landed (tiered-kv's 'built, unwired' table rows: kv/radix now wired via kv/prefixindex, kv/kvtier via model/state/ramspill into serve; the CB memo's kv/budget 'unwired' claim: wired via serving/pipeline + lane_set). - design-continuous-batching: unified scheduler (3 modes, --scheduler), laneSet multi-session owner (2.58x aggregate receipt), weight-read-once GEMM incl. quant + q8-KV — ALL LANDED. Remainder (non-ICB arches, slice-2 prefill admission + batched head, serve rungs, continuity cancel) → task #8. - design-dflash: contract slice + engine forward landed; live lane gated on the non-corrupting aux tap exactly as documented → task #4. - design-tiered-kv: everything landed except caching the declined request classes → task #5. - design-family-roadmap: 26 family lanes landed (ranks 1-11, 13, 14, 18/19 one-offs); the live queue remainder → task #6. - design-quant-formats: research only; the exporter decision + ranked build order → task #7. Full texts remain in git history. Co-Authored-By: Virgil <virgil@lethean.io>
Snider
added a commit
that referenced
this pull request
Jul 22, 2026
….8 tok/s (#338) Second bite on the profiler's #1 kernel: the merge phase still staged all 32 per-lane sorted lists through 8KB of threadgroup memory, a barrier, and a SERIAL lane-0 insertion scan over 128 candidates. Replace it with K rounds of paired simd reductions — simd_max picks the round's best score, simd_min over the max-holders' expert ids applies the same (score desc, index asc) order the insertion sort used, so the selection and the softmax bytes are unchanged. The winner lane shifts its register list down one slot with static K-bounded loops (a data-dependent cursor would re-spill — the exact bug the fc-K fix removed). Receipts (uncached, -count=1 — the metallib is a runtime input go test cannot see, so kernel receipts MUST bypass the test cache): topk 2.44 -> 0.31 ms/token (#1 kernel -> #6); 26B decode tg24 65.0 -> 82.8 tok/s (12.08 ms/token); router parity green; full suite green (107 pkgs). Campaign arc 18.7 -> 82.8, target 112.9. Kernel-sum is now 6.79 ms/token — BELOW MLX's whole-token 8.9 — the remaining gap is per-token host syncs (wait + host embedID; the zero-host chained tail is ICB-gated, which MoE can't use), not kernel math. Co-Authored-By: Virgil <virgil@lethean.io>
Snider
added a commit
that referenced
this pull request
Jul 22, 2026
…nders tasked Audited each design-*.md against the tree (symbols, wiring, receipts), not against its own status lines — two of them undersold what had landed (tiered-kv's 'built, unwired' table rows: kv/radix now wired via kv/prefixindex, kv/kvtier via model/state/ramspill into serve; the CB memo's kv/budget 'unwired' claim: wired via serving/pipeline + lane_set). - design-continuous-batching: unified scheduler (3 modes, --scheduler), laneSet multi-session owner (2.58x aggregate receipt), weight-read-once GEMM incl. quant + q8-KV — ALL LANDED. Remainder (non-ICB arches, slice-2 prefill admission + batched head, serve rungs, continuity cancel) → task #8. - design-dflash: contract slice + engine forward landed; live lane gated on the non-corrupting aux tap exactly as documented → task #4. - design-tiered-kv: everything landed except caching the declined request classes → task #5. - design-family-roadmap: 26 family lanes landed (ranks 1-11, 13, 14, 18/19 one-offs); the live queue remainder → task #6. - design-quant-formats: research only; the exporter decision + ranked build order → task #7. Full texts remain in git history. Co-Authored-By: Virgil <virgil@lethean.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation