Fix/pagi 125 release review findings

[sirozha]

Release: the wire each vendor documents, across every door

fix/PAGI-125-release-review-findings → main-vxcontrol. 421 commits since
v0.1.14-update.7, 418 files, +26102 / −2455. Follow the previous releases and cut the
paired release/… branch from the merge commit.

What this is

Two streams of work. The first closed the review of v0.1.14-update.5..update.7. The second
came out of measuring what actually reaches each vendor: for every door, every model family
and every reasoning-related parameter, what we send was compared against what the vendor
documents, and the differences were fixed. Fifty-two issue keys are referenced across the
range; the composition lives in the two checklists of PAGI-125.

The user-visible shape of it: reasoning is expressed in each vendor's own mechanism instead of
one shape for everyone, sampling is no longer taken away from models whose vendor accepts it,
the output limit travels under the field name each vendor knows, and requests that cannot
work fail locally with a typed error instead of a round trip to a 400.

The compatibility decision — read this first

This release does not preserve full behavioural backward compatibility, and that is
deliberate. Supporting current model generations across every provider was chosen over
keeping the wire byte-identical.

A wire change was accepted only when one of two things held: the old request was rejected by
the vendor anyway, or the change gives the caller back something that used to be dropped
silently. A change that takes away working behaviour is a defect, not a compatibility
trade-off — three such cases were found and two were fixed (the third is a documented
decision about disabling Claude thinking behind an OpenAI-compatible transport).

That criterion was checked, not assumed. A deterministic harness (httptest in place of the
vendor, normalised request bodies, 286 model/option combinations) was diffed against
v0.1.14-update.7: 95 combinations differ, none of them a loss — 34 are the base's own
temperature: 1 that the caller never asked for, 38 were rejected by the vendor anyway, 12
never reached the vendor, 9 restore something to the caller, and 2 turn a vendor 400 into a
local error.

Breaking changes

• WithReasoningDisabled() returns a typed error wherever the disable cannot reach the wire,
  instead of sending nothing or letting the provider answer 400.
• tools/perplexity drops ModelSonarReasoning and ModelR11776 for models the vendor
  retired. Code referencing them will not compile.
• reasoning.ClaudeSupportsEffortWithBudget takes a provider: (model string, p Provider) bool.
• ContentChoice gained Truncated; reasoning.ContentReasoning gained Redacted. Keyed
  literals are unaffected; unkeyed ones no longer compile.
• vertex reports the vendor wire spelling in StopReason.

The full list, with the reasoning behind each, is in the release notes prepared for the tag.

Where the risk is, and how to review it

The diff is large but it is not uniform. Almost all of the behavioural risk sits in two places:

1. llms/reasoning — the tables and predicates that decide, per model name, which
   mechanism reaches the wire. Everything else consults them. Start here.
2. The door adapters — llms/openai, llms/anthropic, llms/bedrock, llms/googleai,
   llms/mistral, llms/ollama. Each door translates the same intent into its own wire form,
   so the same decision can be right in one and wrong in another; the tests are written per
   door for that reason.

The rest is tests, cassettes and documentation. A useful reading order is the fifty contract
decisions recorded in PAGI-125: each says what the library now guarantees and why, and each
maps to a predicate you can grep for.

Verification

• go build ./..., go vet ./..., gofmt -l ., go mod tidy -diff and golangci-lint run
  are clean; all 75 examples build via make build-examples.
• go test ./llms/... — 24 packages green.
• Live acceptance through the pentagi UI after the branch was assembled: eleven doors,
  eighteen models, both the current and previous generation of each vendor. Zero panics, zero
  invariant violations, no error caused by the library.
• Vendor-side behaviour was measured live where it decides a contract — the Bedrock reasoning
  families and the effort levels behind the gateway — and the cassettes were re-recorded from
  those runs, so the recorded traffic matches what the library now sends.
• A public-surface AST diff against the tag reports 2778 exported symbols against 2922 with
  three changes, all listed above. The scan predates the last Bedrock work, which adds one
  more exported name, ContentReasoning.Redacted, also listed above.

Running the tests locally: set placeholder AWS credentials
(AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION) before go test ./llms/bedrock/....
Without them two client-construction tests fail instead of skipping — the httprr guard skips
only when both the cassette and the credentials are missing, and the cassettes are committed.
This is a pre-existing wart, filed for the follow-up patch.

Known red outside llms/: chains and chains/constitution need network, and the
vectorstores/* packages need their databases running. They are red on v0.1.14-update.7 as
well; compare against that baseline rather than against zero.

Not in this PR

The remaining library debt is deliberately left out and collected for the next patch: a
conformance suite for the Bedrock door that exercises the Converse path pentagi actually uses,
the effort-to-budget estimate for high, tool-call argument re-serialisation on the anthropic
door, and three items that never reach a consumer. Each was re-verified against this tree
before being deferred.

Release mechanics

The tag name is not chosen yet and the choice matters: every v0.1.14-* release so far is a
prerelease that go get -u does not select, while v0.1.15 would leave prerelease space and
become the go get -u target for everyone pinned to v0.1.14. Both consumers are currently
on v0.1.14-update.7.

[sirozha]

Automated code review at max effort. This was a single-pass review done without the Agent tool: no multi-agent fan-out and no separate verification pass. I worked through every review angle myself and re-checked each finding against the diff. Several findings were confirmed with throwaway probes against the PR head (355dc71). Those probes are marked "checked against the PR tree" in the comments, and none were committed.

There are 15 inline comments, ranked most severe first: 11 correctness issues, 1 efficiency issue and 3 cleanups.

Several of the findings share one root cause. Vendor-API rules are keyed only on the model name and ignore the host: TakesNoJSONSchema, RejectsPenalties and the Qwen stream-only rule. ServedByDeepSeek(model, host) and DashScopeRoute(model, host) already show the host-aware pattern that would fix all three.

Three findings depend on vendor behaviour that I could not re-measure here: the Gemini penalty 400, the Vertex maxOutputTokens range, and Gemini omitting args on zero-argument calls. Please weigh those accordingly.

---

Generated by Claude Code

[sirozha]

Regression: think is now sent to models that cannot think. resolveThink puts think ("high"/"medium"/"low"/true) on every ReasoningOn request without checking the model's capability. Ollama's /api/chat rejects a truthy think for a model without the thinking capability (server/routes.go in the pinned v0.32.5: "%q does not support thinking", HTTP 400). The base door never set think, so the same call used to succeed.

Scenario: ollama.New(ollama.WithModel("llama3.2")) + GenerateContent(..., llms.WithReasoning(llms.ReasoningHigh, 0)) → the request carries "think":"high" (checked via createChatRequest) → 400. The same happens for gemma3, mistral and other non-thinking models.

Fix: send think only for models known or likely to think (for example reasoning.LikelyReasoningModel, or the capabilities from /api/show). For other models, omit it and add a WarningDrop.

---

Generated by Claude Code

[sirozha]

Regression: filter keys that are not bare identifiers now fail. filterPredicates rejects any key that does not match ^[A-Za-z_][A-Za-z0-9_]{0,62}$. Filters that worked before now return ErrInvalidFilterKey: keys containing -, ., :, spaces or non-ASCII characters, and keys longer than 63 bytes.

Scenario: store.SimilaritySearch(ctx, q, 5, vectorstores.WithFilters(map[string]any{"doc-id": "42"})) → filter key must be a bare identifier: "doc-id". The base produced (cmetadata ->> 'doc-id') = '42' and returned rows.

Keeping the index reachable only requires the key to be inlined as a literal, and quoteLiteral already renders one safely. Rendering cmetadata ->> <quoteLiteral(k)> here and in ddl keeps both the injection fix and the index match, without narrowing which keys are accepted.

---

Generated by Claude Code

[sirozha]

Regression: null arguments are now an error. DecodeFields/Decode reject arguments that are the JSON literal null (ErrNotAnObject). The json.Unmarshal calls they replaced in the googleai and bedrock doors accepted null as an empty map.

The googleai door produces exactly that string itself. json.Marshal(part.FunctionCall.Args) yields "null" when Gemini returns a call without args, as it can for a parameterless function. That case is more likely now that convertTools declares such functions without parameters.

Scenario: on turn 1, Gemini calls a zero-argument tool, so ToolCall.Arguments == "null". On turn 2, replaying that AI message goes through convertParts → toolcall.Decode("null") → toolcall: arguments are not a JSON object: null, and the whole GenerateContent fails. The Bedrock door fails the same way.

Fix: treat a null (or empty) document as an empty object before requiring {.

---

Generated by Claude Code

[sirozha]

Tool messages with any extra part now fail. The loop returns ErrInvalidContentType for any part that is not a ToolCallResponse. The base used Parts[0] and ignored the rest, and the OpenAI door's handleToolMessage explicitly tolerates TextContent in tool messages. A tool message such as [ToolCallResponse{...}, TextContent{Text: ""}], which worked before, now aborts the request with anthropic: invalid content type for tool message.

Fix: skip TextContent parts here (or append non-empty ones as a text block after the tool_result blocks), and return an error only when no ToolCallResponse is present.

---

Generated by Claude Code

[sirozha]

Efficiency: ANALYZE runs on every store start. createMetadataIndexesIfNotExist issues ANALYZE <table> on every New() that declares metadata indexes, not only after an index was created. That contradicts the comment above, which says "every start afterwards is a catalog lookup". The ANALYZE runs inside the init transaction while it holds the embedding-table advisory lock. Each store construction therefore samples the embeddings table, and concurrent constructions queue behind it.

Fix: check to_regclass(<index name>) IS NULL before each CREATE INDEX, and run ANALYZE only when at least one index was actually built. Autovacuum keeps the statistics fresh after that.

---

Generated by Claude Code

[sirozha]

Detached doc comment: HasExplicitTokens and DelegatesDepth were inserted between GetEffort's doc comment and func GetEffort. Godoc now shows the whole GetEffort returns enum value… paragraph as the documentation of HasExplicitTokens, and GetEffort has no documentation. Move the two new methods, with their own comments, below GetEffort.

---

Generated by Claude Code

[sirozha]

Second release review of this PR: multi-agent, ultra depth, on 355dc71.

How it was run. The net diff da7016e..355dc71 (the previous release is exactly the merge base) was split into 14 slices:

• 8 source slices: reasoning tables, openai, anthropic, bedrock, googleai/vertex, mistral/huggingface/ollama, core llms, vectorstores/chains/embeddings. Each was reviewed twice: a first pass, then an independent pass hunting for what the first one missed.
• 4 test-quality slices.
• Test infrastructure and cassettes.
• Cross-cutting checks: public API diff against the base (apidiff), go vet, golangci-lint, go test -race, go mod, examples and docs.

Every finding was then checked by two independent verifiers. One tried to reproduce it with probe tests on HEAD and on the base (through go test -overlay, no files changed); the other tried to refute it. A judge settled disagreements. 117 findings were raised and 106 survived; after merging duplicates across slices, that gives the 87 inline comments below (3 high, 46 medium, 34 low, 4 nit). "pre-existing" marks defects the base already had in lines this PR touches. Findings already posted in the earlier review were not repeated.

Main themes

1. Model-name rules that ignore the host. Rules measured on a vendor's own API also fire on OpenRouter, vLLM, Groq and Together: effort_wire.go lines 29, 49, 76, 180, 211 and 301, off.go 171 and 195, claude_capability.go:49. This is the same root cause as the earlier comments on structured_output.go:235, openaillm.go:311 and effort_wire.go:12. ServedByDeepSeek(model, host) and DashScopeRoute(model, host) already show the host-aware shape.
2. googleai/vertex on the GenAI SDK breaks existing Vertex callers. Credentials plus an API key now fail, and the error prints the key. WithEndpoint in host:port form no longer works, the default location is gone, ClientOptions are dropped in favour of ADC, WithHTTPClient silently loses auth, and exported identifiers were removed.
3. Tests that never run in CI or cannot fail. The new pgvector integration tests and the Ollama Cloud cassette tests always skip, and several assertions cannot fail (see the test comments).

Worth listing in the release notes (API and behaviour breaks)

• vertex exported errors and constants removed.
• perplexity.ModelSonarReasoning and ModelR11776 removed.
• reasoning.ClaudeSupportsEffortWithBudget gained a Provider parameter. This is intentional (d07f16d), so there is no inline comment.
• llmtest.MockLLM.GenerateContentStream removed (intentional, f9ca9d5).
• reasoning.OffEffortNone/OffUnsupported renumbered (iota).
• Bedrock GenerationInfo counters moved from int32 to int, but only partly (see provider_ai21.go:360).
• WithGRPCConn/WithGRPCClient are now refused.
• ValidateReasoning now refuses "none".

Re-check of the earlier review. 12 of its 15 comments were confirmed. Three were not:

• llms/googleai/option.go:52 (16384 default output tokens): the change is deliberate (476f3a6), and the base also always sent a default. The 8k-output models in the scenario (gemini-2.0-flash/flash-lite) have been shut down.
• llms/ollama/ollamallm.go:358 (duplicated gpt-oss mapping): the claimed divergence cannot happen. gptOSSLevel("") is unreachable, because GetEffort never returns "" on the ReasoningOn path, and for every reachable effort both copies agree.
• embeddings/jina/options.go:86 (comment vs BatchSize): the comment is accurate, and the dimension-derived default is unchanged from the base. The real problem nearby is the WithBatchSize(0) panic on line 87.

Those three threads can be resolved.

Checks that passed and limits.

• go vet ./... and golangci-lint v2.12.2 (the CI version) are clean on HEAD.
• go test -race found no data races.
• go mod tidy -diff is clean and all 75 examples build.
• Every changed cassette parses and replays, no cassette is orphaned, and the changed cassettes carry only placeholder credentials.
• There were no live vendor calls (no API keys). AWS, OpenRouter, Pinecone and Perplexity docs were unreachable from the sandbox, so comments that depend on vendor behaviour say so.
• Tests that need Docker did not run. The pgvector findings were checked on a local PostgreSQL 16 + pgvector 0.6.

---

Generated by Claude Code

[sirozha]

[Medium · pre-existing] Cohere batches on Bedrock use the default batch size 512, above the vendor's 96-texts-per-call limit.

Pre-existing, not a regression, but this loop was edited in this PR. With default options, Cohere batches use BatchSize 512, and each batch goes to Bedrock as a single texts array. AWS documents a maximum of 96 texts per call for Cohere Embed on Bedrock. A probe that used WithClient against an httptest endpoint showed NewBedrock(WithModel(ModelCohereEn)).EmbedDocuments(ctx, 200 texts) sending one InvokeModel with 200 texts, the same at HEAD and at base. Against real Bedrock (not tested live) that should return a ValidationException, and no documents get embedded. Suggest capping the batch size at 96 for cohere.* models, e.g. min(b.BatchSize, 96) or a Cohere-specific default, and noting the limit on WithBatchSize.

---

Generated by Claude Code

[sirozha]

[Medium] jina WithBatchSize(0) or a negative value now panics.

Regression: once WithBatchSize is called, applyOptions keeps the caller's value even when it is 0 or negative, and EmbedDocuments then passes it to embeddings.BatchTexts, which divides by it. With NewJina(WithBatchSize(0)), typical for an unset config field, EmbedDocuments(ctx, []string{"a","b"}) panics with integer divide by zero, and WithBatchSize(-1) panics with makeslice: cap out of range. On the previous release both calls embedded the two texts with BatchSize=512. I showed this with an httptest-backed probe run against both HEAD and BASE. Suggested fix: honour the caller only for positive values (o.batchSizeFromCaller && o.BatchSize > 0), or fall back to a sane default before batching.

---

Generated by Claude Code

[sirozha]

[Low · pre-existing] Float filter values in exponent form never match stored JSON numbers.

Non-string filter values are bound as fmt.Sprintf("%v", v). For float64, %v switches to exponent form (from 1e21 up and below 1e-4), while the json column holds what encoding/json wrote, and ->> returns that text. Every number in a filter decoded from a JSON request body is a float64. So {"user_id": 1234567} binds "1.234567e+06" against a stored 1234567, and {"ts": 1727500000} binds "1.7275e+09". SimilaritySearch then returns 0 documents with no error, while the same filter with an int matches. I confirmed the mismatch with a probe that compares filterPredicates' bound value with json.Marshal of the same value. The previous release never matched non-string values either, so this is an incomplete fix rather than a regression, but commit ec0ee19 advertises that such filters now match. Suggested fix: render numbers the way encoding/json does, e.g. strconv.FormatFloat(v, 'f', -1, 64) for floats, or json.Marshal for any non-string scalar.

---

Generated by Claude Code

[sirozha]

[Nit] ExtraBody drop warning names a vendor SDK the Anthropic door does not use.

The anthropic door's reason for dropping WithExtraBody says "the door builds its request through a vendor SDK and has nowhere to merge them". That is not true for this door: internal/anthropicclient marshals its own messagePayload with encoding/json, and llms/anthropic imports no vendor SDK (unlike ollama, which uses ollama/api). The string looks copied from the SDK-based doors. So WithExtraBody(map[string]any{"metadata": ...}) on anthropic returns a drop warning that blames an SDK that isn't used, and suggests the field can't be supported when the door could actually merge it into the body it builds. Suggest either giving anthropic its own accurate reason (for example "the door does not merge extra body fields yet") or actually merging ExtraBody into the marshalled payload, for both the messages path (line 15) and the completions path (line 172).

---

Generated by Claude Code

[sirozha]

[Nit] Two doc comments sit on the wrong functions after the refactor.

This doc comment names converseToolChoice but is attached to carriesToolBlocks, which does something else: it scans the history for toolUse/toolResult blocks, and line 177 uses it to keep the tool config even when the choice is "none". converseToolChoice (line 774) is left with no comment. The same thing happened at line 838: // processStreamingResponse processes streaming events now documents the newly inserted deliverToolCall, and processStreamingResponse (line 867) has none. You can see both by reading HEAD lines 760-783 and 838-867; new functions were inserted between each comment and the function it names. Fix: move each comment back above its function and give carriesToolBlocks and deliverToolCall their own comments (for example, say that tool blocks in the history require a toolConfig on Bedrock, which is why "none" falls back to auto).

---

Generated by Claude Code

[sirozha]

[Nit] Stale comment states the opposite of the renamed test's assertion.

The comment above this test is stale: it still says adaptive reasoning with no effort "defaults to high, matching the Anthropic/Bedrock paths". The renamed test now asserts the opposite, failing if any reasoning_effort is sent, which is the behaviour a459111 introduced (the vendor picks its default). The test passes at 355dc71, so the comment, not the assertion, is wrong. A maintainer who trusts the comment could restore reasoning_effort: high for delegated requests, bringing back the most expensive effort that a459111 removed. Suggest rewording it to something like: "WithAdaptiveReasoning with no explicit effort hands the depth decision to the model: no reasoning_effort is sent, so the vendor default applies."

---

Generated by Claude Code

[sirozha]

[Nit] InferenceSpeed doc points to Speed "above", but Speed is declared below.

Nit: this comment says "Not to be confused with Speed above", but the TTS Speed field is declared further down in CallOptions (line 348, under // TTS options.). Please change "above" to "below", or just name the field ("the TTS Speed field").

---

Generated by Claude Code