Skip to content

Commit c73cc35

Browse files
committed
List what the branch review left open in TODO.md
A new section collects the points the review fixes left open or the reviewers raised outside their findings' scope, grouped by area: the Ollama fields still dropped on /api/generate and the non-text prompt answered as a load; a backend IOException after the stream started; the open decision whether a malformed stream envelope should fail rather than end the stream, with both reviewers' positions; the chat mapper's remaining loose reads (a non-numeric max_tokens, n or n_probs, the unreachable logprobs-with-tools refusal, Anthropic's named tool choice and other tool_choice object types); the Anthropic image source and media-type questions; the missing or environment- bound tests; and an agent options copy that drops fields. The deliberate deviations the fixes chose are listed once so they are not rediscovered. Documentation only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBFFnrhtvXiUeyxjmUc4R2
1 parent 96a82d6 commit c73cc35

1 file changed

Lines changed: 99 additions & 0 deletions

File tree

‎TODO.md‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,105 @@ they are not rediscovered:
523523
Ollama itself unloads there and answers `done_reason "unload"`; this server does not claim an
524524
unload it did not do.
525525

526+
### Open points from the branch review of 2026-10-20/21 (details: `BUGHUNT-FOUND.md`)
527+
528+
The review of the bug-hunt branch confirmed its findings; the fixes are F213 … F243 and the dated
529+
addenda in the ledger. What the fixes left open, or what the reviewers raised outside the findings'
530+
scope, is listed here; the ledger entry in brackets holds the analysis. Delete an item when it is done.
531+
532+
**Ollama routes**
533+
- **`/api/generate` with `raw: true` drops `format` and `images` without a word** (F215). Ollama keeps
534+
both. Either map them (`format` as `json_schema`, `images` as multimodal data, as the non-raw path
535+
does) or refuse them with `400`; today neither happens.
536+
- **`/api/generate` without `raw` drops `template` and `context` without a word** (F215); Ollama
537+
applies them. The chat-template path has no slot for a caller's template or a token context, so the
538+
honest options are a `400` or a documented deviation.
539+
- **A non-text `prompt` (`["a"]`, `{}`) on `/api/generate` is answered `200` with `done_reason
540+
"load"`** (F205): `isLoadOnly` treats every non-textual prompt as absent. Ollama's field is a string;
541+
the fix is a `400` for a present, non-null, non-text prompt (`requireTextualPrompt`), with a
542+
model-free `OllamaApiSupportTest` case and an HTTP test, red/green.
543+
544+
**Server transport**
545+
- **After a stream started, a backend `IOException` is taken for a disconnect** (F214): it is logged
546+
at DEBUG and the stream ends without an error event. A third-party `OpenAiBackend` may report a
547+
generation failure that way; a quiet error event (`writeQuietly` of the route's error event) would
548+
cost nothing when the client really is gone. Optional, outside what F182 promised.
549+
- **Decision open: should a malformed stream envelope fail instead of ending the stream?**
550+
`ChatStreamChunkParser.feed` treats an unparseable or empty envelope as the end of the stream, and
551+
`StreamingChunkAssembler` follows it, so a broken envelope looks like a short answer. One reviewer:
552+
the path is not reachable from `LlamaModel`, whose native side always writes a well-formed envelope,
553+
so hardening buys nothing. The other: throw an `IllegalArgumentException` naming the envelope
554+
instead of a silent end, as the rerank and tool-schema readers now do (F232, F233). Decide, then either pin the
555+
current contract or change it with a red/green test.
556+
557+
**Chat mapper (`OpenAiRequestMapper`, every chat route)**
558+
- **A non-numeric `max_tokens` (`"100"`) is read as `-1`, i.e. no limit** (F223 names it): upstream's
559+
schema answers `400`. The Anthropic route now refuses it before the mapper; the OpenAI route itself
560+
does not.
561+
- **A non-numeric `n` or `n_probs` (a string) is ignored** where upstream's schema answers `400`.
562+
Pre-existing, not a regression of the branch.
563+
- **Upstream's "logprobs is not supported with tools + stream" refusal is unreachable** (F220): the
564+
mapper turns `logprobs` into `n_probs` and does not see `stream`.
565+
- **Anthropic `{"type":"tool","name":…}` still means `required` over all tools, and a `tool_choice`
566+
object of another type (`allowed_tools`, `mcp`, …) falls back to `auto` without a word** on the chat
567+
and Responses paths (F227). The named-function narrowing of F227 is the model for the first; the
568+
second needs a decision between `400` and a documented fallback.
569+
570+
**Anthropic Messages**
571+
- **The image source `file` (Files API) is dropped without a word**, here and upstream (F29). Pin it
572+
in a test (`aFileImageSourceIsDroppedLikeUpstream`) or refuse it; a decision first.
573+
- **A `base64` image without `media_type` defaults to `image/png` here and to `image/jpeg` upstream**
574+
(F29); neither aligned nor pinned by a test. A decision first.
575+
- **Upstream candidate:** `stop_reason` `stop_sequence` for a matched stop word (F221) is one line in
576+
each of upstream's `to_json_anthropic*` plus an assertion in `test_compat_anthropic.py`; add it to
577+
the upstream inventory below for the day after the release.
578+
579+
**Decided deviations, recorded so they are not rediscovered** (no action): unknown part types inside
580+
a Responses `message` are not refused, because the same branch carries assistant `output_text` /
581+
`refusal` parts (F224); upstream's stricter Responses role checks are not adopted (F228); a boolean or
582+
string rerank `top_n` and one outside the `int` range keep every result, where upstream converts
583+
`true` to 1 and wraps the cast (F230); a `documents` of the wrong type stays `400` rather than
584+
falling back to `texts` (F231); `chat_template_kwargs` keeps its string fallback, because a string
585+
literal is a valid kwarg value while a tool schema must be an object (F233); a stop issued before
586+
the serving thread has entered `jllama_rpc_server_serve` cannot be told from "nothing running" and
587+
stays a no-op -- callers repeat the stop until the serve returns, as `RpcServer` does (F241).
588+
589+
**Tests**
590+
- **The model-backed `ErrorHandlingTest` case for a 4-byte character in a refusal is missing**:
591+
`handleChatCompletions` with a `response_format` of type `😀` must throw `InvalidRequestException`
592+
with `but got: 😀` in the message. The model-free half (a failed `LlamaModel` call keeps a 4-byte
593+
character) is pinned.
594+
- **The argument prefix that arrives before a tool call's name never reaches `onPartialToolCall`**
595+
(`StreamingChunkAssembler`; documented in its Javadoc since this pass). If it is fixed, the contract
596+
assertion `assertPartialsMakeUpTheCompleteCall` in `toolCallDeltaBeforeItsName_doesNotFailTheStream`
597+
lands in the same commit; today it is red (`Expected: is "{\"city\":1}" but: was "ty\":1}"`).
598+
- **`LoraCacheFreshSlot` writes `slot0.bin` into the test's working directory**, not under its
599+
`--slot-save-path`: `engine::slot_action` uses the file name as the path, as
600+
`LlamaModel.saveSlot(int, String filepath)` documents. Probably the test should pass a path under
601+
the save directory; needs a decision.
602+
- **`RpcClient.AStaleServerIsKeptOutOfALaterLoadThatDidNotAskForIt` fails under
603+
`--gtest_repeat=2`** in one process (also filtered alone); it passes in a normal `ctest` run. The
604+
test drives ggml's process-wide registry directly; find out which state the first run leaves.
605+
- **Three FIM tests fail against a model without FIM tokens** (`500`):
606+
`OpenAiServerCompletionIntegrationTest#infillReturnsContent`, `#ollamaGenerateWithSuffixUsesInfill`
607+
and `ErrorHandlingTest#testHandleInfillMissingInputSuffix`. Not a defect -- CI's model has the
608+
tokens -- but they should self-skip on a model without them, as the model-gated tests do.
609+
- **`MemoryManagementTest` (context 32) assumes a context shift** that llama.cpp no longer makes by
610+
default (`ctx_shift = false` at b11639); check whether it still tests what it says, and use the new
611+
`enableContextShift()` if it should.
612+
- **`llama-ffm`'s `FfmRpcServerTest` has no natives assumption**: without a native library it fails
613+
instead of skipping like the module's other tests.
614+
- **The model-backed rerank tests were not run locally** (no reranker model in the sandbox):
615+
`OpenAiServerRerankIntegrationTest` covers F230 … F232 end to end only in CI, and F177's caveat that
616+
its end-to-end case had not run locally can go once a CI run with the jina reranker shows it green.
617+
- **The agent's 4xx path is tested without retries**: the tests run with `RetryPolicy.NONE`, so
618+
nothing pins that Atmosphere skips retrying a `4xx` (optional).
619+
620+
**Atmosphere agent**
621+
- **`AgentOptions.withWorkspace` copies only `baseUrl`, `modelPath` and `workspace`**: the copy falls
622+
back to the default `modelId` and every other option's default. Noticed while fixing F243, not
623+
changed.
624+
526625
### The release: 6.0.0, with an "Upgrading from 5.1" section (decided 2026-10-15; roadmap item 9)
527626

528627
**Nothing has been published since 5.1.0 on 2026-08-29.** `pom.xml` is `6.0.0-SNAPSHOT`, and

0 commit comments

Comments
 (0)