feat: B6 — first-class OSS and local model support

[zjshen14]

Closes #296. Part of #251. Design doc: docs/design/b6-oss-models.md.

Makes Ollama (local), Kimi, GLM, DeepSeek, Qwen, and OpenRouter first-class. The B1 transport plumbing already shipped and openai.ts was already a generic chat-completions client — so these models were reachable. What was missing is that nothing knew about them.

The four defects this fixes

#	Defect	Effect
1	detectProvider() returned "gemini" for unrecognised names	--model kimi-k3 silently built a Gemini client
2	Context windows fell back to 100_000	Hosted OSS models ship 1M → auto-compact fired 13x too early. Local models are usually smaller (stock qwen2.5-coder:14b = 32 768) → auto-compact never fired and the model silently truncated
3	resolveApiKey() was a hardcoded 3-way branch	A Moonshot key had to live in OPENAI_API_KEY, colliding with real OpenAI
4	hasNativeThinking() only matched Gemini	Always-on-reasoning models still got a redundant think tool

Defect 2 is the one worth pausing on: the local direction is the dangerous one, because over-estimating the window means no compaction warning ever appears — the context just quietly falls off the end.

What landed

src/providers/registry.ts — one table owning wire format, base URL, API-key env vars, context windows, and capabilities. detectProvider(), contextWindowFor(), hasNativeThinking(), and resolveApiKey() all read from it. Adding a provider is now a data change, not four call-site edits.

src/providers/ollama-discovery.ts — queries /api/tags for a local model's real context window, and warns at startup when the selected model can't call tools. Best-effort throughout: Ollama being down degrades to static defaults rather than crashing.

src/providers/salvage.ts — recovers tool calls that open-weight models emit as text.

Why salvage is not optional

Verified against qwen2.5-coder:14b on Ollama 0.32.5. The chat template instructs the model to wrap calls in <tool_call> tags. It emits bare JSON instead:

{ "name": "ls", "arguments": { "path": "/tmp" } }

Ollama's parser finds no tags and leaves it in message.content with finish_reason: "stop", tool_calls: null. This reproduces identically on the native /api/chat and the OpenAI-compatible endpoint — so it is a model instruction-following gap, not a bug in Ollama's translation layer or in OpenCLI. The agent loop sees zero calls and ends the turn, meaning local support would connect successfully and then do nothing.

Safety rails: salvage runs only when a turn produced no structured calls, and only promotes a name that was actually offered in that request. A model discussing a tool in prose is never silently executed. Opt-in per preset, so first-party providers are untouched.

Incidental hardening

The OpenAI client is now more tolerant of compatible servers generally: tool calls are flushed at end-of-stream (many such servers report finish_reason: "stop" even with calls pending, which previously stranded them), and one malformed argument blob no longer aborts the whole stream and discards well-formed calls alongside it.

Drive-by fix

#251 — config.temperature was loaded into Config but never reached the client unless --temperature was passed explicitly, so the configured default was dead.

Verification

Real agent turn against local qwen2.5-coder:14b, no API key, no network:

{"type":"llm_call_start","model":"qwen2.5-coder:14b","inputMessages":1}
{"type":"tool_exec_start","name":"read","args":{"file_path":"sample.txt"}}
{"type":"tool_exec_end","name":"read","latencyMs":2,"success":true}
The file `sample.txt` has 4 lines.

Salvage fires, the tool executes, the agent answers. (The count is wrong — the file has 3 lines. That is 14B-model accuracy, not an OpenCLI defect; the loop, tool execution, and result handling are all correct.)

Context-window resolution against the live instance:

contextWindowFor("qwen2.5-coder:14b") WITHOUT discovery: 100000
contextWindowFor("qwen2.5-coder:14b") WITH discovery   : 32768

817 tests passing (+83). New coverage: registry lookup/prefix ordering, Ollama discovery (including connection-refused, malformed JSON, missing fields, cache behaviour), salvage (bare / fenced / tagged / arrays / split-across-chunks, plus every must-not-promote case), and the client-level integration.

Not in scope

Architect/editor routing (B5) — depends on B4 (#63, AgentContext as a serializable value type). This registry is a prerequisite for it, not a delivery of it.

🤖 Generated with Claude Code

[zjshen14]

Review

Read the design doc first, then the code. I agree with the design — collapsing four parallel tables into one keyed table is the right shape, and the priority argument for local-first is correct. Ran the full gate locally on fc03d49: typecheck, lint, format:check clean, 817 passed / 31 skipped.

The framing I most agree with is §"Context windows wrong in both directions": over-estimating a local window is the dangerous direction because nothing surfaces it — no warning, no error, just context falling off the end. Prioritising that over the hosted 1M presets is the right call. §4 is also genuinely well-evidenced: reproducing the bare-JSON emission on both /api/chat and /v1/chat/completions is what turns "Ollama bug?" into "model instruction-following gap", and that distinction is what justifies putting salvage in our client rather than waiting upstream.

Below: two places the doc claims more than the code delivers, then code findings. Nothing here is a blocker.

---

Design doc vs. implementation

1. Salvage is on for six presets, the doc says one. The §1 preset table marks salvage on only for ollama, and §4 closes with "opt-in per provider, so first-party providers are entirely unaffected". The code enables salvageToolCalls: true for ollama, moonshot, zai, deepseek, dashscope, and openrouter (registry.ts). First-party is indeed unaffected, so the letter holds — but the doc reads as "local only" and it isn't. It isn't free either (see finding 2). Either widen the doc's table or narrow the presets; my preference is narrowing to ollama + openrouter (the gateway can front local-quality weights), since Kimi/GLM/DeepSeek/Qwen hosted endpoints emit structured calls properly.

2. §4's safety claim is stronger than the guard actually is. The doc says the name-must-match check is "what makes this safe rather than reckless". That check does not cover the case where the model outputs the JSON for a real tool as an illustration. Verified against the built code:

content: {"name":"bash","arguments":{"command":"rm -rf /tmp/x"}}   tools offered: [bash, read]
salvageToolCalls() -> [{"name":"bash","args":{"command":"rm -rf /tmp/x"}}]   contentIsOnlyToolCalls: true

That gets promoted to a function_call and the text is suppressed. What actually protects us is two things the doc doesn't credit:

• the whole-content requirement — parseCandidate() bails unless the cleaned content starts with {/[, so any leading prose defeats salvage. Verified: "Sure, here is what a call looks like:\n{...}" → []. This is the real guard, and it's a good one.
• the HITL confirmation gate, which a salvaged bash/write still passes through like any other call.

Worth restating §4 accurately — the doc is the artifact future readers will trust, and "name check makes it safe" invites someone to later relax the start-of-content check thinking the name check has them covered.

---

Code

3. detectProvider() still misroutes local model names — the half the doc calls priority. Defect #1 is fixed for hosted names but the fallback is unchanged for exactly the names a local user types. Verified:

--model	detected	result
qwen2.5-coder:14b	dashscope	"No DashScope API key found"
gpt-oss:20b	openai	routed at api.openai.com
llama3.1:8b, mistral-small, codellama	gemini	silent wrong client — defect #1 verbatim

--provider ollama is required and documented, so this is UX not correctness, and I agree detectPrefixes on the ollama preset is wrong (local names are arbitrary). But the silent gemini fallback is the failure the doc opens with, still present. Cheap fix: when a model name matches no prefix, write one stderr line naming the fallback and --provider. A name:tag shape → suggest ollama hint would be better still. The comment at registry.ts:218 justifies keeping the fallback; it doesn't argue for keeping it silent.

4. Salvage buffering holds an entire legitimate JSON answer to end-of-stream. couldBeToolCall() returns true forever for content starting with { or [, so "write me a tsconfig" against any of the six salvage presets streams nothing and lands as one block at the end. Correctness is fine — the text is emitted, and the prose-release path is well tested (openai.salvage.test.ts:91, :124) — but it's a real streaming regression on the exact providers where responses are slowest. Bounding the hold-back (release once buffered exceeds what a plausible tool call could be, say a few KB) would cap the damage without weakening salvage. This is the strongest argument for narrowing the preset list in finding 1.

5. contextWindowFor() searches every provider's table, and the miss is silent and in the dangerous direction. compact.ts:108 calls findModelInfo(model) with no providerId, even though the CLI knows the provider at construction. Today's prefixes mostly don't collide, but: run glm-5.2 locally on Ollama and have discovery come back empty (Ollama still starting, fronted by a proxy, /api/tags blocked) — resolveContextWindow() returns undefined, and the model inherits Z.ai's 1 000 000 rather than the 100k default. Auto-compact never fires. That is precisely the invisible failure §"both directions" is about, reintroduced through the cross-provider search. findModelInfo() already takes the scoping parameter; it just needs Agent to receive provider and pass it through.

6. The one piece with no unit test is the piece that joins the other three. resolveContextWindow() (index.ts:317) is where override → discovery → static precedence actually happens, i.e. where defect #2's local half is really fixed. Registry, discovery, and salvage each have thorough tables; this doesn't, because it's stranded in index.ts. It's already nearly pure (model, provider, baseUrl, config) — lifting it next to keys.ts would make the precedence chain testable, including the "discovery unavailable" path from finding 5.

7. Nit: drainCalls() swallowing a JSON parse failure into args = {} is the right call over aborting the stream, but the model then gets a "missing required parameter" error with no trace of why. One stderr line under --debug would make that debuggable.

---

Things I'd call out as good

• Threading baseUrl into createCompactionClient() fixes a real latent bug, not just an OSS one: any --base-url session was previously compacting against the vendor's cloud endpoint with a key scoped to the proxy.
• The end-of-stream drainCalls() flush for servers that report finish_reason: "stop" with calls pending — that would have stranded tool calls on plenty of OpenAI-compatible servers, unrelated to salvage.
• Not caching a failed Ollama discovery (ollama-discovery.ts:96) so a late-starting daemon isn't poisoned for the session, and returning undefined from toolSupportWarning() for unknown models rather than warning on absence of evidence. Both are the restrained choice.
• PRESETS integrity tests (registry.test.ts:12) — asserting every non-first-party OpenAI-wire preset declares a base URL is the invariant that keeps "adding a provider is a data change" honest as the table grows.

Housekeeping

Per CLAUDE.md, the design doc's _Status: must not read "Ready for implementation" once the code is on main — flip b6-oss-models.md:3 to Implemented — merged in <sha> (<date>) in this PR or immediately after merge.

Verdict: approve with the above as follow-ups. Findings 1 and 2 are doc edits I'd do before merge since they're a few lines; 3–7 are fine as issues.

[zjshen14]

Thanks — this was a genuinely useful review. I verified every finding against the built code before acting; all seven reproduced exactly as described. Addressed in 132ec67.

I took six of them and pushed back on one. Taking the disagreement first.

Finding 1 — I disagree, and kept salvage on all OSS presets

The argument for narrowing is that Kimi/GLM/DeepSeek/Qwen hosted endpoints emit structured calls properly. That is plausible, but neither of us verified it — I have no keys for those services, and the review does not cite a test either. So narrowing rests on the same class of unverified assumption as leaving it on. That makes it a question of which error is recoverable:

	Cost
Enabled but unnecessary	Bounded buffering (now capped, per finding 4), plus a misfire risk that is structurally tiny — the payload must be the entire response and carry a name matching an offered tool. A JSON answer like a tsconfig has no such field.
Disabled but necessary	The agent loop sees no calls and silently does nothing — the headline bug this PR exists to fix.

The asymmetry favours keeping it on, especially now the cost is bounded. I have recorded that reasoning in the PRESETS doc comment rather than leaving it implicit, and pinned the policy with a test so a future change to the salvage set has to be deliberate rather than incidental. If a hosted endpoint is ever shown to always emit structured calls, its flag is a one-line change.

Your underlying point — that the doc claimed one thing and the code did another — was completely right, and that half is fixed.

Finding 5 — the best catch, and worse than described

Verified:

contextWindowFor("glm-5.2")                    -> 1000000
contextWindowFor("glm-5.2", undefined, "ollama") -> 100000   (after fix)

A local glm-5.2 with discovery unavailable inherited Z.ai's 1M. You called this "the invisible failure reintroduced through the cross-provider search" and that is exactly right — it defeated the specific thing the milestone exists to prevent. Agent now takes provider and scopes the lookup.

Finding 2 — you were right, my doc credited the wrong mechanism

Reproduced your case exactly:

content: {"name":"bash","arguments":{"command":"rm -rf /tmp/x"}}  offered: [bash, read]
-> salvaged: [{"name":"bash","args":{"command":"rm -rf /tmp/x"}}]
with leading prose                                              -> []

So the name-match check is not what makes this safe. §4 now credits the three things that actually do — the whole-content requirement, the no-structured-calls precondition, and the HITL gate — and explicitly names the residual risk we accept. Your reasoning for why this matters (that a future reader might relax the start-of-content check believing the name check covers them) is precisely why I wrote it as a warning rather than a footnote.

Findings 3, 4, 6, 7 — taken

3. All three rows of your table now name the real cause:

--model qwen2.5-coder:14b -> warn: looks like a local Ollama model ... pass --provider ollama
--model llama3.1:8b       -> warn: looks like a local Ollama model ... pass --provider ollama
--model mistral-small     -> warn: matches no known provider prefix; defaulting to 'gemini'

Agreed the fallback should stay and only the silence was wrong. The name:tag shape turned out to be a reliable local signal, so that hint is in too. Also fixed the --provider help text, which still listed only the three first-party providers.

4. Bounded at 32 KB — sized to still admit a write call carrying a file body, which is the case that ruled out the "few KB" figure.

6. Lifted to src/cli/context-window.ts with 10 tests, including the discovery-unavailable path that must return undefined rather than invent a number. That path is now explicitly asserted, since guessing high there is the dangerous direction.

7. Reported through an injected onWarn sink rather than a direct write, since core/ and providers/ must not touch stderr.

Housekeeping

Set the _Status:_ line to Implemented — under review in #297, with a note to flip it to the merge SHA on merge. I did not write a SHA I cannot know yet; happy to follow up immediately after merge.

---

842 tests passing (+25). Re-verified end-to-end against local qwen2.5-coder:14b — salvage still fires, read still executes.

[zjshen14]

Re-review of 132ec67 — approving

Re-ran the gate on 132ec67: typecheck, lint, format:check clean, 842 passed / 31 skipped. CI green on all three checks, mergeStateStatus: CLEAN.

On finding 1 — you're right, I withdraw it

The argument that changed my mind is the one I should have applied to myself: my narrowing suggestion rested on "hosted OSS endpoints emit structured calls properly", which I asserted and did not test. That is the same class of unverified assumption as leaving the flag on, so "narrow it" was not the conservative choice I presented it as — it was a different guess with a worse failure mode. Enabled-but-unnecessary costs bounded buffering; disabled-but-necessary is a silent no-op agent. The asymmetry decides it.

Pinning the policy with salvage policy > enables salvage for every OSS/local preset and no first-party one is what makes this durable — the decision now has to be un-made deliberately. That's better than either of the outcomes I proposed.

Findings 2–7

All verified in the code, and three landed better than I framed them:

• 4 — 32 KB sized to admit a write carrying a file body is the right derivation. My "few KB" would have silently killed salvage for the single most valuable call type; that's a case where the number mattered more than the mechanism, and you picked it from the workload rather than from feel.
• 6 — it("returns undefined rather than inventing a number") is the assertion I actually wanted and didn't articulate. The precedence chain matters less than the guarantee that a discovery failure can never manufacture a large window.
• 7 — injecting onWarn instead of taking the stderr write I suggested. My suggestion would have put a direct process.stderr write inside providers/, which the layering rules forbid; you took the finding and rejected the implementation, correctly.

contextWindowFor("glm-5.2", undefined, "ollama") -> 100000 closes finding 5 at the root, and the providerDetectionWarning output now names the real cause on all three rows of my table.

Two leftovers — both small, neither blocking

A. §4's closing line contradicts §1. It still reads:

Salvage is opt-in per provider (ollama, openrouter — see §1), so first-party providers are entirely unaffected.

§1 now says salvage is on for all six OSS presets and argues for it at length. The parenthetical looks like a leftover from a partial edit toward my original suggestion. It's one line, but it's the same doc-says-one-thing/code-does-another divergence that finding 1 was about, so it's worth not landing on main.

B. src/eval/replay/runner.ts:87 is the last unscoped contextWindowFor path. The Agent there is constructed without provider, so replay still falls through to the all-providers findModelInfo search. A tape running model: "glm-5.2" gets 1M rather than the scoped value, which shifts the auto-compact ratio — so a tape exercising compaction thresholds could pass in replay and behave differently in production. Low stakes since tape authors pick their own model names, but it is the one place the finding-5 fix doesn't reach.

Housekeeping

_Status:_ reading Implemented — under review in #297 was the right call — writing a SHA you can't know yet would have been worse. I'll flip it to the merge SHA in the post-merge follow-up CLAUDE.md requires, and fold leftover A into that same commit. B I'll leave as a follow-up issue rather than expanding this PR.

Merging. Good disagreement — the one finding you pushed back on is the one where you were right.