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}
…ashFile The pack-copy filesystem helpers were byte-equivalent to go-mlx's private copies but unexported on both sides — Go visibility blocked the engine deleting its 173-line duplicate. Exported as thin delegates with the established names; the existing Good/Bad/Ugly triplets repoint at the exported surface (internals unchanged, so coverage carries). Second of the two visibility unlocks flagged by the go-mlx DRY wave (the first was the quantise kernel dispatch, 68e67a5). Co-Authored-By: Virgil <virgil@lethean.io>
GeneratorCredit on ModelBookOptions — empty keeps the byte-identical default colophon credit, non-empty substitutes through the same xmlEscape as every body string (retires go-mlx's colophon patch). dataset.BatchConfig becomes the canonical home for the quartet (BatchSize/MaxSeqLen/SequencePacking/NoEOS — same JSON tags, so the checkpoint wire format is untouched); distill.BatchConfig is a true alias with a compile-time proof. eval's opaque BatchConfig left alone deliberately. safetensors exports Float16ToFloat32/BFloat16ToFloat32 as documented wrappers over the internal engines (DecodeFloat32 dogfoods them) — lets go-mlx delete its exported duplicate. Full module 4090 green / 74 packages; vet + gofmt clean. Co-Authored-By: Virgil <virgil@lethean.io>
…harness fully proven state (68->100): the package-level Store dispatchers had 0% (only benchmarks touched them) — Resolve/Borrow/MergeRef now exercised through minimal interface-probe fakes; InMemoryStore's guards (nil-ctx, cancelled-ctx, nil-receiver, ref backfill, zero-value self-init) and the ProjectSeed comparators' full switch matrices covered directly. eval (72->100): the full RunDataset branch matrix (missing hooks, adapter nil/error, empty batches, NaN/Inf loss, all-skipped), quality-probe fallbacks, and the scoring helpers. Zero production changes, zero seams. Race-clean -count=2. Full-module gate deliberately deferred to the orchestrator (siblings mid-edit). Co-Authored-By: Virgil <virgil@lethean.io>
…peline driven for real 294 tests. The deep gap was agent_eval: processMLXNative and processWithConversion had never survived past adapter-convert failure — a fileWritingTransport now writes genuine safetensors via modelmgmt.WriteSafetensors and a fake Ollama+judge server carries both through fetch -> MLX->PEFT conversion -> model creation -> probing -> judge scoring -> Influx push. RunAgentLoop's near-duplicate triplet redesigned into three distinct scenarios. Worker heartbeat/poll/register/infer and the SSH arg-composition family covered directly. Residual ~3% itemized honestly: live-SSH success branches, non- OneShot loop tails (goroutine leak to test), /etc/machine-id on macOS, stdlib request-construction failures. Zero production changes. Race-clean. Full-module gate deferred to the orchestrator (providers agent still mid-flight). Co-Authored-By: Virgil <virgil@lethean.io>
…fault-injection to the floor 706 tests race-clean across the pair. ollama: the three 0% size estimators cross-checked against AppendX via a never-undercounts contract; direct UnmarshalJSON invocation bypasses encoding/json's checkValid pre-scan to reach every hand-rolled decoder branch. openai: outbound HTTP fault injection (stub round-trippers, erroring readers, truncated bodies), streaming envelope, thinking-extractor edges, resolver/stops/handler families. Remaining openai fraction is traced-dead defensive arms, itemised in the reports. Twin-dispatch note: an interrupted launch double-fired this brief; the two agents converged on one tree and the second caught two real test defects the first introduced — accidental peer review, kept. Zero production changes. Full-module gate follows as the orchestrator step. Co-Authored-By: Virgil <virgil@lethean.io>
…pe with llama_ftype Upstream ggml.h numbers the per-tensor enum GGML_TYPE_MXFP4=39 and GGML_TYPE_NVFP4=40 (36-38 are the removed IQ4_NL_4_4/4_8/8_8 slots); the separate llama_ftype FILE-type enum uses 38/39 for the same quant families, and the constants here had taken the file-type numbering. A real GGUF tensor tagged type 39 (genuine MXFP4) was reported as NVFP4, and type 40 (genuine NVFP4) fell off the details table and read as unknown. Verified against ggml-org/llama.cpp master ggml.h (fetched, not assumed). Twin of the identical fix in go-mlx's go/gguf (aca31ba7 there) — found by the go-mlx wave-2 test-audit fleet, which escalated the shared-package copy. New regression pin asserts ggml_type 38 is unknown and 39/40 are mxfp4/nvfp4, keyed by literal upstream IDs so a wrong constant can no longer drift with its own table row. Receipts: gguf 112 tests pass (-count=1, incl. the new pin); vet + gofmt clean. Co-Authored-By: Virgil <virgil@lethean.io>
…sition reconcile
Resolves the open call in go-mlx/docs/MIGRATION.md ("reconcile go-mlx
composition into serving's shape, or the reverse"): the inference contract
layer is the boundary, engines self-register behind inference.LoadModel,
and the go-mlx composition core dissolves (contract ADDs up, registration
glue into engine/metal, metal-typed remainder dies with pkg/metal).
Carries the Tier-0 contract diff (imports-verified type-by-type table of
metal.X vs inference.X), per-file destinations for the composition core,
and the four execution tiers.
Co-Authored-By: Virgil <virgil@lethean.io>
…engine-merge Tier 1) Two contract ADDs from the Tier-0 diff in docs/engine-merge.md — the engine-neutral homes for what the go-mlx composition core reads from metal.MTPMetrics and metal.DeviceInfo today: - SpeculativeMetrics + SpeculativeMetricsProvider: draft/verify counters, rates, timing, and peak memory for one speculative-decode operation (gemma4 MTP on metal now, hip/cuda lanes share the shape). Probed off a TextModel like AttentionInspector. - DeviceInfo + DeviceInfoProvider + BackendDeviceInfo(name): accelerator description behind a registered backend, per-backend dispatch mirroring SetRuntimeMemoryLimits. One test per symbol (Good/Bad/Ugly + examples), root suite green -count=1. Co-Authored-By: Virgil <virgil@lethean.io>
The engine-agnostic KV state layer — blocks, snapshot, analysis, state-store — lifted verbatim from go-mlx/go/kv (MIGRATE-UP per go-mlx/docs/MIGRATION.md). Zero import rewrites: the package already consumed inference/state, inference/state/filestore and inference/safetensors; core is its only other dependency. Suite green in the new home (-count=1). Left behind with intent: bench.go + its tests — the kv bench harness is the package's one mlx/memory consumer and follows when memory (with its profile/pack chain) migrates. Unblocks: KV-snapshot + session capability contracts in kv.Snapshot terms (engine-merge.md Tier 1), and go-mlx #259 (pkg/native speaks kv.Snapshot natively — kvconv retires). Co-Authored-By: Virgil <virgil@lethean.io>
…engine-merge Tier 1) KVSnapshotter / KVChunkSnapshotter / KVRestorer / PromptCacheWarmer / PromptCacheChunkWarmer / PromptCacheClearer — the engine-neutral homes for the composition core's native* probe interfaces, expressed directly in kv.Snapshot (the portable wire shape) instead of a per-engine snapshot type. The four-way plain/WithOptions split collapses into one options struct per capture (zero value = default capture). This is the contract half of go-mlx #259: when pkg/native implements these, kvconv (DIES-WITH-METAL per the migration map) has nothing left to convert. Block-source restore is deliberately deferred until the native implementation fixes its real shape. One test per symbol + example, root suite green -count=1. Co-Authored-By: Virgil <virgil@lethean.io>
…cution map spine splits by file, not package: prompt/token/tokenizer are zero-dep (lifting now), spine.go IS the GenerateConfig home (root aliases it) so the config reconcile and the spine lift are one work item, model_info waits on memory, lora_config/metal_convert ride into engine/metal. session hangs on kvconv retirement (#259), the SessionHandle re-home, and the agent/artifact/blockcache chain — per-file blocker table added. Records the in-flight wave-A lifts (bundle; probe + blockcache + spine core files) running in isolated worktrees. Co-Authored-By: Virgil <virgil@lethean.io>
…e spine delta (engine-merge Wave B) spine.GenerateConfig vs inference.GenerateConfig: 13 of 18 fields were already identical. The root config grows the delta — Thinking (the resolved thought-channel policy), TraceTokenPhases, TraceTokenText, GenerationClearCache/-Interval — plus WithThinking. EnableThinking (API intent) and Thinking (engine policy) coexist by design: serving resolves one into the other. The thinking trio hoists from parser into the root as ThinkingConfig / ThinkingMode / ThinkingChunk with parser type-aliasing back (zero consumer breakage): parser imports the root for Token and the parse-result contracts, so GenerateConfig could not reference parser.Config without a cycle. Remaining for this reconcile: ProbeSink once the probe lift merges; spine.go's option conversions re-point at Tier 3. Root + parser suites green -count=1; whole module builds. Co-Authored-By: Virgil <virgil@lethean.io>
Moves the 10-file bundle package (portable model-state artifact: kv snapshot + tokenizer/runtime/adapter/sampler identity, plus the SAMI visualisation conversion) from go-mlx/go/bundle into go-inference/go/bundle unchanged in behaviour. Import rewrites (mechanical): dappco.re/go/mlx/kv -> dappco.re/go/inference/kv (already lifted) dappco.re/go/mlx/lora -> dappco.re/go/inference/lora lora reconcile: bundle's only cross-package dependency on lora is lora.AdapterInfo (7 fields: Name, Path, Hash, Rank, Alpha, Scale, TargetKeys; 2 methods: IsEmpty, Identity). go-mlx's own lora package already declares `type AdapterInfo = ilora.AdapterInfo` — a type alias onto dappco.re/go/inference/lora.AdapterInfo — so the two types are identical, not just structurally compatible. No literal syntax or field adjustments were needed beyond the import path swap. Verification: go.work in this worktree could not resolve (external/* submodules uninitialised here, though pinned to the same SHAs as the main go-inference checkout), so go vet/test/build were run with go.work temporarily repointed at the main checkout's already-populated external/* absolute paths (verification-only; go.work reverted to its original relative-path form before this commit, and is not part of this diff). go vet ./bundle -> clean go test ./bundle -count=1 -> ok dappco.re/go/inference/bundle 0.220s (95 tests) go build ./... -> clean (whole module) grep -rn "dappco.re/go/mlx" go/bundle/ -> zero hits Co-Authored-By: Virgil <virgil@lethean.io>
Copied all 7 files from go-mlx/go/probe verbatim (probe.go, influx.go + tests/benches/examples). Only rewrite: example-test self-imports dappco.re/go/mlx/probe -> dappco.re/go/inference/probe in probe_example_test.go and influx_example_test.go. Deps are core + dappco.re/go/io only. Test receipt: ok dappco.re/go/inference/probe 0.329s (go test -count=1); go vet clean; grep dappco.re/go/mlx in probe/ -> zero hits. Co-Authored-By: Virgil <virgil@lethean.io>
Copied all 6 files from go-mlx/go/blockcache verbatim (blockcache.go + tests/benches/examples/branch/disk suites). Zero rewrites needed — imports were already inference-native (dappco.re/go/inference, dappco.re/go/inference/state, core) and the package has no mlx self-import. Test receipt: ok dappco.re/go/inference/blockcache 0.573s (go test -count=1); go vet clean; grep dappco.re/go/mlx in blockcache/ -> zero hits. Co-Authored-By: Virgil <virgil@lethean.io>
Copied only the 9 zero-dependency vertebrae from go-mlx/go/spine:
prompt.go + prompt_{test,bench_test,example_test}.go, token.go,
tokenizer.go + tokenizer_{test,bench_test,example_test}.go. Excluded
spine.go, model_info*, lora_config*, metal_convert* (later
non-mechanical wave). Only rewrite: example-test self-imports
dappco.re/go/mlx/spine -> dappco.re/go/inference/spine in
prompt_example_test.go and tokenizer_example_test.go. Verified the 9
files are self-contained — no references to symbols in the excluded
files.
Test receipt: ok dappco.re/go/inference/spine 0.802s (go test -count=1);
go vet clean; go build ./... clean across the module; grep
dappco.re/go/mlx in spine/ -> zero hits.
Co-Authored-By: Virgil <virgil@lethean.io>
…mplete this side of Tier 3 The last spine-delta field: research-telemetry sink (attention/logit probe events) now that the probe lift is merged. probe imports only core+coreio, so the root import is cycle-safe. inference.GenerateConfig now carries every field spine.GenerateConfig declares; the Tier-3 move re-points spine's aliases and conversions, nothing more. Co-Authored-By: Virgil <virgil@lethean.io>
Copies go-mlx/go/artifact (artifact.go + 3 test files) into go-inference/go/artifact unchanged apart from import rewrites: dappco.re/go/mlx/bundle -> dappco.re/go/inference/bundle, dappco.re/go/mlx/kv -> dappco.re/go/inference/kv. The dappco.re/go/inference/state import was already correct in the source and needed no change; no other dappco.re/go/mlx/* imports were present (no self-import rewrite applied — all 4 files are `package artifact`, not an external test package). Gates: go vet ./artifact clean, go build ./... clean, gofmt -l artifact/ empty, zero dappco.re/go/mlx hits under artifact/. Test receipt: ok dappco.re/go/inference/artifact 0.205s (7 tests). Co-Authored-By: Virgil <virgil@lethean.io>
… zero cgo, pkg/metal deleted never ported Two Snider corrections folded in: - "metal" in engine/metal names the Apple GPU API the pure-Go engine drives (tmc/apple bindings) — NOT go-mlx's pkg/metal, which is deleted wholesale, never ported. The payload is pkg/native and only pkg/native. - There is NO cgo anywhere in what moves: no module-boundary requirement; darwin/arm64 build tags gate it in the main module. Substance fixes that fall out: anything expressed in pkg/metal's cgo graph ops cannot "ride" anywhere — eval.go/model_lora.go graph work and spine's lora_config/metal_convert are DIES-WITH-METAL; native train/eval is future feature work in pkg/native, not a port. Co-Authored-By: Virgil <virgil@lethean.io>
file-presence flagged serve.go with no test/example sibling at all; ax7-gaps flagged all nine exported symbols (RunServe, the seven ServeOption setters, Serve). ServeOption setters are exercised directly against serveConfig; Serve/RunServe get real end-to-end boot+shutdown coverage against a live listener (freeListenAddr + waitForHTTPUp), including a layered-options case proving the admin Bearer wall never leaves an admin route unauthenticated while inference stays open. Examples reuse the pre-cancelled-context boot/shutdown path for a fast, deterministic Serve/RunServe demonstration (verified stable over 30 repeated runs). Co-Authored-By: Virgil <virgil@lethean.io>
…let convention RenderGemmaToolDeclarations/RenderGemmaToolCall/ParseGemmaToolCalls (#300) already had strong, real coverage but under names ax7-gaps couldn't tie to their symbol. Split the single RenderGemmaToolCall inline Good/Bad/Ugly test into three named functions (bodies unchanged), split RenderGemmaToolDeclarations into Good/Bad + a new multi-tool/multi-property Ugly case, and rename the three ParseGemmaToolCalls scenario tests onto Good/Bad/Ugly (MixedArgs kept as-is, supplementary). Add gemma_tools_example_test.go. Co-Authored-By: Virgil <virgil@lethean.io>
ax7-gaps flagged DraftDetection.Active, DetectGemma4DraftPath and ResolveServeDraft missing the naming triplet. New triplet tests add real new coverage of previously-untested reactive-ladder rungs (MTP/ single-gguf, the ambiguous-MTP-dir fallthrough, and the sibling mtp-*.gguf rung) rather than just renaming the existing scenario tests. Adds serve_draft_example_test.go. Co-Authored-By: Virgil <virgil@lethean.io>
file-presence flagged serve_resolver.go missing an example sibling (the
ModelLoader/SpeculativeLoader type declarations count as public symbols
for that check even though ax7-gaps/example-gaps have nothing to demand —
they're func types, not funcs/methods). Adds Example{ModelLoader,
SpeculativeLoader} showing how RunServe's engine-injection seams are
implemented and invoked.
Co-Authored-By: Virgil <virgil@lethean.io>
TestBackendLlama_LlamaBackend_LoadModel_Bad,
TestBackendHttpTextmodel_HTTPTextModel_Classify_{Bad,Ugly} and
TestBackendMlx_NewMLXBackend_{Good,Bad,Ugly} asserted real behaviour
already but were thin one/two-liners. Each now adds a genuine additional
assertion (error shape, lastErr side-effect, OK-false) rather than padding
with no-op lines.
Co-Authored-By: Virgil <virgil@lethean.io>
ax7-gaps requires TestArtifact_ExportArtifact_{Good,Bad,Ugly} (file-prefix
naming) — the existing tests were correct in substance, just named
TestExportArtifact_* without the file prefix. Rename only, bodies
unchanged.
Co-Authored-By: Virgil <virgil@lethean.io>
- Rename every store_test.go triplet to the required Store_ file prefix (Resolve, ResolveBytes, ResolveRefBytes, BorrowBytes, BorrowRefBytes, ResolveURI, MergeRef, ChunkNotFoundError/URIChunkNotFoundError Error/Unwrap). - Fill the genuinely missing Good/Bad/Ugly variants — notably the pass-through-vs-sanitise asymmetry each dispatcher has on its resolver error path (Resolve/BorrowBytes/BorrowRefBytes/ResolveURI pass a dirty resolver return straight through; ResolveBytes/ResolveRefBytes zero it instead), an inconsistent-overlay case for MergeRef, and Unwrap Good/Bad/Ugly (nil receiver, boxed-interface errors.Is survival). - Add store_example_test.go — one Example per exported func/method, required by example-gaps and previously absent for this file. Co-Authored-By: Virgil <virgil@lethean.io>
…standard
memory.go:
- Rename InMemoryStore method triplets to the required
Memory_InMemoryStore_<Method> shape (Get/Resolve/ResolveBytes/
BorrowBytes/BorrowRefBytes/ResolveURI/Put/PutBytes) — the receiver
("InMemoryStore") doesn't match the file prefix ("Memory"), so the
shorthand form doesn't apply and the full form was missing throughout.
- Add the missing NewInMemoryStore/NewInMemoryStoreWithManifest variants,
a Get triplet (previously untested directly), and the
live-view-vs-defensive-copy contrast between Borrow*/Resolve* that the
doc comments promise but nothing exercised directly before.
- Add memory_example_test.go (was entirely absent).
state_test.go dissolved: it matched no production file (no state.go
exists), so none of its tests counted under the file-aware ax7-gaps
convention regardless of what they covered. Its unique content is now
folded in under the correct file — the InMemoryStore construction/binary/
borrow-aliasing checks into memory_test.go, and the Session/Forker
contract check into the new agent_memory_test.go.
agent_memory.go (Ref/WakeRequest/WakeResult/SleepRequest/SleepResult are
data-only; Session/Forker are the only behavioural contract):
- New agent_memory_test.go: Forker/Session Good/Bad/Ugly (happy path,
failing-fork nil-return contract, AgentMemory* alias interchangeability).
- New agent_memory_example_test.go (file-presence requires it; no
ax7/example-gaps symbol demand since these are types, not funcs).
Co-Authored-By: Virgil <virgil@lethean.io>
identity.go is pure JSON-tagged data (ModelIdentity, TokenizerIdentity, AdapterIdentity, RuntimeIdentity, SamplerConfig, StateRef, Bundle) with no funcs/methods of its own, so ax7-gaps/example-gaps have no symbol-level demand here — but file-presence still requires both sibling files since these are public types. Cover the wire-shape contract that matters: omitempty collapsing zero values, the deprecated MemvidRefs field surviving independently of its StateRefs replacement, and the StateBundle/Bundle alias identity. Co-Authored-By: Virgil <virgil@lethean.io>
- Split NewProjectSeed's combined test into Good (explicit input,
verbatim) / Bad (untrimmed, slash-padded input — proves cleanURI/Trim
normalise it) / Ugly (degenerate all-defaults case).
- Add WakeRequest Bad/Ugly: Bad documents a real corner of
setProjectLabel — a seed with no Labels of its own and no caller
Labels ends up with wake.Labels == nil, not {project_id: ...}; Ugly
proves the caller's own project_id label is never overwritten.
- Add PlanContinuation Bad: an unrecognised Mode value is coerced to
state-checkpoint persistence by the switch's default arm.
- Split the combined CheckWakeCompatibility test into Good/Bad/Ugly
(compatible+warning / two independently-incompatible requests /
SkipCompatibilityCheck short-circuiting the whole evaluation).
- Add project_seed_example_test.go (was entirely absent).
Co-Authored-By: Virgil <virgil@lethean.io>
model_test.go gets full Good/Bad/Ugly coverage for every exported TextModel symbol (NewTextModel, OpenTrainer, Generate, Chat, FormatChatPrompt/ Continuation, Classify, BatchGenerate, NewSession, ModelType, Info, Capabilities, Metrics, Err, Close) plus direct tests for the unexported helpers (stopTokens, tokenInSet, chatTurnRole, prefillSplit), backed by a shared fakeSession/fakeTokenModel double and a real (LoadTokenizer-loaded) fixture tokenizer, and an enginetest.TextModel conformance run. trainer.go has no funcs of its own (interfaces only) — trainer_test.go pins the Trainer/TrainerModel contract shape via fakeTrainer/fakeTrainerModel, which model_test.go's OpenTrainer tests also drive. Renamed a handful of pre-existing tests (Capabilities, FormatChatPrompt/Continuation) to the required TestModel_TextModel_<Symbol>_<Variant> naming so ax7-gaps stops flagging symbols that already had real coverage under a legacy name. Co-Authored-By: Virgil <virgil@lethean.io>
session_test.go gets full Good/Bad/Ugly coverage for every exported SessionHandle symbol (NewSessionHandle, Prefill, AppendPrompt, Generate, CaptureKV, RangeKVBlocks, RestoreKV, RestoreFromKV, Fork, Reset, Close, Err), using the fakeSession/fakeTokenModel doubles from model_test.go plus an enginetest.SessionHandle conformance run. Fork's Ugly case needed a sequenced-session fixture (fakeTokenModel.sessions) so the fork target can fail its restore independently of the parent session. The Err_Ugly case corrected an initial wrong assumption: a clean Close does NOT clear a prior recorded failure (only the engine session's own Close error would overwrite it) — the test now pins that real, slightly surprising behaviour instead of a false expectation. Co-Authored-By: Virgil <virgil@lethean.io>
vision_test.go (new) covers TextModel.AcceptsImages Good/Bad/Ugly plus direct tests for the unexported helpers vision.go carries: chatMultimodal (the image-turn path Chat routes to — a full success walkthrough plus a table of every distinct failure branch: project error, zero soft tokens, empty/ mismatched placeholder run, splice failure, session-open failure, a session missing the VisionSession seam, and a failing embeddings prefill), plus countTokenID, messagesHaveImages and modelSampleParams. New fakeVisionTokenModel/fakeVisionSession doubles layer the vision seams over the existing fakeTokenModel/fakeSession; the fixture tokenizer's one-char "z" vocab entry makes an image placeholder block tokenise to an exact, countable run, so the placeholder-count invariant is exercised for real rather than asserted by construction. Co-Authored-By: Virgil <virgil@lethean.io>
…ion/sdpa/rope cluster)
DiffusionSDPA had zero direct test coverage under AX-7's file-aware naming
(only an existing Good-shaped test lived in diffusion_test.go, which maps to
diffusion.go's symbols, not diffusion_attention.go's). Adds
TestDiffusionAttention_DiffusionSDPA_{Good,Bad,Ugly} plus ExampleDiffusionSDPA.
…sdpa/rope cluster) AudioAttention/AudioAttentionF32 had no direct test file at all. Adds Good/Bad/Ugly triplets for both entry points (cross-checked against each other; length-guard error paths; the ChunkSize=1/Past=0/Future=0 degenerate window that collapses to Post(VProj(x)) exactly) plus targeted unit tests for the pure-Go windowing helpers (audioContextSizeOf, audioBlockContextF32, audioBlockedMask, audioRelShiftF32) and the two Example tests.
…lBF16 (attention/sdpa/rope cluster) Corrects a stale header comment that pointed at a "mtp_attn_metal_test.go" byte-identical cgo oracle test — pkg/metal was retired, so that oracle no longer exists. Adds a host causal-softmax reference (sdpaCausalBF16Reference) as the real correctness gate, the causal-boundary proof (perturbing a future-only key must not change a query's output), the dimension/GQA guard error paths, and the undersized-output-capacity fallback, plus Example tests for both entry points.
… (attention/sdpa/rope cluster)
Renames the existing identity/shape-mismatch/backing-reuse tests to the
required Test<Symbol>_{Good,Bad,Ugly} shape (bodies unchanged) and adds the
missing variants: a convention-agnostic norm-preservation proof for the
traditional=true pairing at a nonzero offset, RoPEInto's shape guard, and
the undersized-output-capacity fallback. Plus Example tests for both.
…qsBF16/Into (attention/sdpa/rope cluster)
Renames the existing base/non-plain/backing-reuse tests to the required
Test<Symbol>_{Good,Bad,Ugly} shape (bodies unchanged) and adds the missing
variants: the rotaryDim/invFreqs spectrum-length guards for both entry
points and the undersized-output-capacity fallback for RoPEFreqsBF16Into.
Plus Example tests for both.
…RopeBF16/Into (attention/sdpa/rope cluster)
Renames the existing parity/backing-reuse tests to the required
Test<Symbol>_{Good,Bad,Ugly} shape (bodies unchanged) and adds the missing
variants: shape/headDim-cap guards for both entry points, the headDim==512
boundary (proving the cap still works AT the limit, not just erroring above
it), and the undersized-output-capacity fallback for Into. Plus Example
tests for both.
…edBF16/Into (attention/sdpa/rope cluster)
Renames the existing contiguous-reference/backing-reuse tests to the
required Test<Symbol>_{Good,Bad,Ugly} shape (bodies unchanged) and adds the
missing variants: sdpaPagedValidate's GQA/length/page-count/alignment guard
paths for both entry points, the MQA-with-a-single-one-row-page edge, and
the undersized-output-capacity fallback for Into. Plus Example tests for
both.
…s families (attention/sdpa/rope cluster)
Renames the existing single-value/GQA-guard/backing-reuse tests to the
required Test<Symbol>_{Good,Bad,Ugly} shape (bodies unchanged) and adds the
missing variants: a real multi-key GQA correctness proof against an
independent host softmax reference for SDPA, the undersized-output-capacity
fallback for SDPAInto, and — SDPA2Pass/SDPA2PassInto had no triplet at all
under this file's AX-7 naming (their only prior coverage lived in
sdpa_2pass_test.go, whose "sdpa_2pass" stem doesn't match sdpa.go) — a
single-pass cross-check, the GQA guard, the batched (b>1) two-pass path,
and the undersized-output-capacity fallback. Plus Example tests for all
four symbols.
…nBlock/Into (attention/sdpa/rope cluster)
Renames the existing composed-primitives/backing-reuse tests to the
required Test<Symbol>_{Good,Bad,Ugly} shape (bodies unchanged) and adds the
missing variants: attentionBlockInto's per-input length guards for both
entry points, a real GQA (nHeads>nKVHeads) + longer-cache + nonzero-offset
extension of the composed-primitives proof, and the undersized-output-
capacity fallback for AttentionBlockInto. Plus Example tests for both.
…ter)
sdpa_multiq.go had zero test coverage (no exported symbols, so the
file-presence/ax7-gaps analysers don't flag it, but the batched multi-query
causal SDPA fold is real, non-trivial GPU dispatch logic). Adds pipeline
resolution Good/Bad cases and a row-by-row cross-check of the K-query
causal dispatch against K single-query encSDPAStrided dispatches over the
same seq-major cache — the fold's whole correctness claim ("byte-identical
per row to K single-query dispatches").
… cluster) sdpa_multiq_ring.go had zero test coverage (no exported symbols). Adds pipeline-resolution Good/Bad cases for the ring SDPA and copy kernels, a per-element identity + offset-copy proof for encCopyBF16Contig (including its missing-library error path), and a fresh/empty-ring (ringLive=0) degenerate-case proof for encSDPAMultiQRing that reduces the two-segment attention to a plain causal fold over the staged rows — cross-checked against encSDPAMultiQCausal. Manually verified the ring test discriminates (forcing ringLive nonzero drops the cross-check cosine to ~0.66) before keeping it.
|
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