From 94f934bf0665e9e69f363de2caf4ca3e3afe669a Mon Sep 17 00:00:00 2001 From: hellodk Date: Fri, 28 Aug 2026 14:37:33 +0530 Subject: [PATCH 1/5] docs: implementation plan for guided setup, context truth, decode profiles, prefill Adds the full design for removing the legacy champ.* settings layer in favour of a single guided setup wizard, plus runtime context-window truth, task-aware decode profiles, and prefill/structured-output workstreams. Ticket map: #118-121. --- docs/IMPLEMENTATION_PLAN.md | 467 ++++++++++++++++++++++++++++++++++++ 1 file changed, 467 insertions(+) create mode 100644 docs/IMPLEMENTATION_PLAN.md diff --git a/docs/IMPLEMENTATION_PLAN.md b/docs/IMPLEMENTATION_PLAN.md new file mode 100644 index 0000000..f87a57f --- /dev/null +++ b/docs/IMPLEMENTATION_PLAN.md @@ -0,0 +1,467 @@ +# Champ — New Implementation Plan: Guided Provider Setup, Context Truth, and Decode/Preflfill Intelligence + +**Status:** Proposed · **Target:** v1.8.0 +**Owner:** champ core maintainers +**Related docs:** `docs/CONFIG.md`, `docs/ARCHITECTURE.md`, `docs/DESIGN_SMART_MODEL_ROUTING.md` +**Ticket map:** see [Tasks](#10-task-breakdown--tickets) + +--- + +## 0. TL;DR + +Champ's configuration and inference layer is being rebuilt around four workstreams. Configuration moves to **YAML-only**: the legacy flat `champ.*` VS Code settings are removed, and every entry point (status bar, chat-header gear, first-run) now launches **one guided setup wizard** that walks the user through provider → endpoint → API key → **live model auto-detection** → config write, with keys always stored in `SecretStorage`. + +**Workstream A — Guided provider setup.** Replace "open VS Code settings" with a wizard. Pick provider (local vs cloud), endpoint (prefilled, probe-able), API key only when required (cloud, or any `401` from a self-hosted server), models **auto-discovered live** from the endpoint and pre-selected for the user. Result is written (merged, never clobbered) into `.champ/config.yaml` or `~/.champ/config.yaml`. Legacy `champ.*` provider/model/endpoint/key settings are deleted from the manifest, along with the dead settings-loading code path. + +**Workstream B — Context-window truth.** Champ currently budgets prompts against a model's *theoretical* context length, which silently over-subscribes when a local backend serves a smaller window. B changes discovery + the provider layer to read the *runtime* window (`num_ctx` / `n_ctx`) with a strict fallback ladder, stores the resolved value, and splits each request into `input ≤ window − output-reserve`. Every downstream budget (compaction, teams, RAG packing) is corrected automatically by one shared source of truth. + +**Workstream C — Decode parameters.** Today every request is `temperature 0.7`, fixed, for *every* task. C introduces task-aware decode profiles (tool-call vs code-gen vs chat vs autocomplete), routing through the existing model classifier, with per-provider field mapping and per-provider YAML overrides (`options:` block). + +**Workstream D — Prefill & structured output.** Two improvements: (1) *assistant-prefill* — seed the assistant turn with the tool-call preamble so local models never narrate before calling tools; (2) *structured output* — grammar/JSON-schema-constrained generation where the backend supports it, making malformed tool calls structurally impossible. Companion requirement: keep the prompt prefix byte-stable so backend KV/prompt caches hit on multi-turn agent loops. + +Each workstream is an independent ticket with its own failing tests. Commits are made per ticket; the full unit suite is the merge gate. + +--- + +## 1. Background & Motivation + +### 1.1 Current configuration surface (to be replaced) + +Today Champ reads provider configuration from four layers in priority order: + +``` +workspace YAML > user YAML > VS Code champ.* settings > built-in defaults +``` + +The settings layer (`champ.provider`, `champ.ollama.baseUrl`, `champ.openaiCompatible.apiKey`, `champ.configSource`, …) is legacy and now actively harmful: two competing configuration vocabularies, a `configSource` knob to arbitrate them, and users unknowingly configuring the extension in *both* places. The settings layer dies. + +### 1.2 Why a wizard, why not a settings page + +The chat-header gear and status bar currently do *different things* (one opens VS Code settings for `champ`, the other opens/creates `config.yaml`). New users land in either and see an empty editor. The replacement is a **single, guided command** (`champ.configureProvider`) that produces a *valid, working* config every time: + +1. It knows the provider catalogue (local vs cloud, default endpoints). +2. It probes the endpoint before writing — bad URLs fail fast with a fix. +3. It auto-detects models live instead of asking the user to know model IDs. +4. It prompts for API keys at exactly the right moment, and only when needed. +5. It writes a merged YAML file the rest of Champ already understands. + +The wizard is native (QuickPick / InputBox), no webview work required, and reuses the existing file-watcher hot reload. + +### 1.3 Competitor landscape + +A leading competitor in the local-model space ships a provider extension that plugs models into an editor's native chat via a language-model provider API. Its advantages over Champ's current plumbing are narrow and specific — not the agent loop, not approvals, not RAG — but four of them are worth stealing: + +1. **Runtime context reporting.** It queries each model's *served* context window (the `num_ctx` value baked into the running model, which reflects actual VRAM/RAM-allowed capacity), not the model's advertised maximum. The difference is large and consequential: an 8B model advertises a 128k window but typically serves 4k–32k on typical hardware. Champ uses the advertised maximum today → requests over-subscribe → backends hard-fail (`request exceeds available context size`). +2. **Input/output budget split.** It reports input and output limits as separate numbers derived from one shared window, so the caller never double-spends. +3. **Self-calibrating token estimation.** It adapts its chars-per-token ratio from observed usage per model instead of a fixed estimate. +4. **Picker-vs-server diagnostics.** A command that cross-checks what the UI offers against what the backend actually serves. + +Champ already beats that competitor on the agent side (tools, approvals, multi-agent, RAG checkpoints). What it needs is the **context plumbing**. + +### 1.4 Documented pain points this plan fixes + +| Problem observed in the field | Root cause today | Fix | +|---|---|---| +| "Prompt too long / exceeds context" errors on local models | Budget = 75% of *advertised* window, not *served* window | B | +| Wizard-less setup → broken or half-configured installs | Config written by hand, no validation | A | +| Same 0.7 temperature for tool calls and creative chat | Single fixed decode profile | C | +| Local models narrate "I'll now search…" and never emit a tool call | Prompt-only tool calling, no prefill or grammar | D | +| Slow multi-turn agent loops on local hardware | No KV/prompt-cache reuse because prefix isn't stable | D | + +--- + +## 2. Design: Workstream A — Guided Provider Setup (wizard + YAML-only) + +### 2.1 Entry points → one command + +``` +status bar click ──────┐ +chat-header gear ──────┼──▶ champ.configureProvider ──▶ wizard flow +"Champ: Configure Provider" ─┘ +first-run detection ────────┘ +``` + +`champ.openSettings` keeps its palette entry (command continues to exist) but its *behavior* is repurposed to launch the wizard. The old `workbench.action.openSettings("champ")` call is removed. `champ.setApiKey` is subsumed into the wizard (it stays registered for backward-compat, but the wizard calls it for the key step). + +### 2.2 Wizard state machine + +``` + ┌──────────────────────────────────────────────────────┐ + │ champ.configureProvider │ + └──────────────────────────────────────────────────────┘ + │ + ┌─────────────────────────────┴─────────────────────────────┐ + │ Step 0. Scope │ + │ workspace open? │ + │ Y → "Workspace (.champ/config.yaml)" [recommended] │ + │ "User (~/.champ/config.yaml)" │ + │ + (if target exists) "Edit existing config" │ + │ N → user scope (no choice) │ + └─────────────────────────────┬─────────────────────────────┘ + │ + ┌─────────────────────────────┴─────────────────────────────┐ + │ Step 1. Provider (QuickPick, grouped) │ + │ ── Local (key optional) ── │ + │ "Local · " (server detected @ port X) │ + │ "Local · " (not reachable) │ + │ "Local · OpenAI-compatible (custom endpoint)" │ + │ ── Cloud (API key required) ── │ + │ "Cloud · Claude" (key saved ✓ / key needed) │ + │ "Cloud · OpenAI" (…) │ + │ "Cloud · Gemini" (…) │ + └─────────────────────────────┬─────────────────────────────┘ + │ + ┌────────────────────┴────────────────────┐ + │ cloud provider? │ + │ ┌──────┴──────┐ │ + │ yes no │ + │ │ │ │ + │ │ ▼ │ + │ │ Step 2. Endpoint (InputBox) │ + │ │ prefilled default per backend │ + │ │ "Probe endpoint" → /v1/models │ + │ │ 200 → proceed (green) │ + │ │ 401 → ask for key (Step 3) │ + │ │ 000/refused → error + retry │ + │ │ openai-compatible: empty start │ + │ │ │ │ + │ ▼ ▼ │ + │ Step 3. Key │ │ + │ key stored? │ │ + │ Y → skip │ │ + │ N → password │ │ + │ InputBox │ │ + │ secrets.store │ + │ (401-driven for local) │ + │ │ │ + └─────────────┼────────────────────────────┘ + ▼ + ┌─────────────────────────────────────────────┐ + │ Step 4. Model auto-detect + pick │ + │ live discovery against chosen endpoint: │ + │ providers[].listModels() │ + │ → native /api/tags │ + │ → /v1/models (OpenAI-compat) │ + │ → /props (n_ctx world) │ + │ "[scanning models…]" progress │ + │ QuickPick of discovered models │ + │ unreachable → "configure anyway (manual │ + │ model) | Retry | Back" │ + │ cloud + key already present → AUTO-KEEP │ + └──────────────────────┬──────────────────────┘ + ▼ + ┌─────────────────────────────────────────────┐ + │ Step 5. Write + reload │ + │ merge into chosen scope YAML, preserving │ + │ existing keys (never clobber) │ + │ provider: │ + │ providers.: {baseUrl, model, …} │ + │ file watcher → loadProvider() → status │ + │ toast: "Champ: configured → " │ + └─────────────────────────────────────────────┘ +``` + +**Key prompting rules (per earlier decisions):** + +- **API key prompt cadence.** + - Cloud provider: prompt iff no key in `SecretStorage` (`champ.{provider}.apiKey`). + - Cloud provider + key already present: **auto-keep** the current model, skip Step 4 pick. + - Self-hosted: key prompt only when the endpoint probe returns **`401`**. Never pre-emptively. +- **Model pick.** + - Local: always run live discovery against the endpoint; user picks from results. + - If discovery fails (server unreachable), offer `"configure anyway (manual model)" | "Retry" | "Back"`. +- **Discovery endpoints** (see [3.1](#31-context-window-reconciliation--the-source-of-truth)) reuse the existing smart-router fetch ladder. + +### 2.3 YAML writer: merge, never clobber + +New pure helper `src/config/yaml-writer.ts`: + +``` +writeProviderToYaml(existingText | null, scope, { providerId, baseUrl, model, options }) +``` + +- Parses existing YAML → object. +- Sets `provider` (top-level), `providers..{baseUrl, model}`. +- Deep-merges `options` from Workstream C if present. +- Round-trips the rest of the file (autocomplete, indexing, agent, mcp) untouched. +- Falls back to writing a fresh template when the file is absent or empty. + +Unit tests: preserve unrelated keys, handle empty/absent file, handle `provider` already set, list-scope idempotency. + +### 2.4 Removal of the legacy settings layer + +| Remove from `package.json` `contributes.configuration` | Reason | +|---|---| +| `champ.provider` | YAML `provider:` | +| `champ.claude.model`, `champ.openai.model`, `champ.gemini.model` | YAML `providers.*.model` | +| `champ.ollama.baseUrl/model`, `champ.llamacpp.baseUrl`, `champ.vllm.baseUrl/apiKey`, `champ.openaiCompatible.*` | YAML `providers.*` | +| `champ.autocomplete.model`, `champ.autocomplete.provider` | YAML `autocomplete.*` | +| `champ.indexing.embeddingProvider` | YAML `indexing.embeddingProvider` | +| `champ.configSource` | settings layer deleted; single-source YAML | +| `champ.openaiCompatible.apiKey`, `champ.vllm.apiKey` | `SecretStorage` via wizard | + +| Keep (runtime/UX, not provider-model config) | +|---| +| `champ.yoloMode`, `champ.autoFix.*`, `champ.indexing.enabled`, `champ.autocomplete.enabled`, `champ.autocomplete.debounceMs`, `champ.mcp.servers`, `champ.autoContext.enabled`, `champ.userRules` | + +**Dead code to delete** (with their tests): + +- `ProviderFactory.createFromConfig` + all `createClaude/OpenAI/Gemini/Ollama/LlamaCpp/VLLM/OpenAICompatible(config, secrets)` settings-reading methods + the `ConfigReader` interface → keep only `createFromChampConfig`. +- The `"settings"` source in the layered resolver. +- The `onDidChangeConfiguration → loadProvider()` watcher for provider keys. +- Branches in `loadProviderInner` that pick the settings vs YAML path. + +**Migration safety net:** on activation, if stale `champ.provider` / `champ.ollama.baseUrl`-style keys are detected in `settings.json`, show a one-time `"Export to YAML & clear" | "Dismiss"` notice. "Export" converts the flat keys to a `.champ/config.yaml` and clears the settings keys; carried in the same tickets as a small command + tests. + +--- + +## 3. Design: Workstream B — Context-Window Truth + +### 3.1 Context-window reconciliation: the source of truth + +A single pure function resolves a model's *effective served* context window from the richest available metadata. All consumers read from it. + +``` + resolveEffectiveContextWindow(metadata) → number + + priority ladder (first hit wins): + 1. runtime parameter parameters.num_ctx // what the running + 2. runtime modelfile modelfile::num_ctx // instance actually serves + 3. show-response show.{max_context_length, context_length} + 4. tags details tag.details.context_length + 5. model_info model_info["*.context_length"] + 6. fallback provider-specific default (e.g. 8192) +``` + +``` +FACTS: + advertised (theory) = model_info schémata *.context_length e.g. 262144 + served (runtime) = parameters.num_ctx e.g. 16384 + ───────────────────────────────────────────────────────────────────────── + Champ today budgets 75% × 262144 = 196608 ❌ exceeds 16384 + Champ after (B) input ≤ 16384 − outputReserve ✅ +``` + +### 3.2 Where it is wired + +``` + ┌──────────────────────────────────────────────────────┐ + │ Discovery (before) Discovery (after) │ + │ listModels → id/name listModels + hydrate │ + │ fallback /api/tags /api/tags w/ details │ + │ fixed 8192/4096 ladder → real window │ + └──────────────────────────────────────────────────────┘ + │ + ▼ + ┌──────────────────────────────────────────────────────┐ + │ modelInfo().contextWindow = resolved served window │ + │ providers..contextWindow (persisted in YAML) │ + └──────────────────────────────────────────────────────┘ + │ + ┌───────────────┬───────────────────┴────────────┐ + ▼ ▼ ▼ + context-manager provider.trimForContext team-agent + (agent budget) (per-request trimming) (team token budget) +``` + +- **Provider layer.** The `modelInfo()` calls that currently return `8192`/`4096` hardcodes read the resolved value. +- **Smart-router discovery.** The `/api/tags` and `/v1/models` discovery paths populate `contextWindow` from the resolved ladder instead of constants. +- **Persistence.** The wizard (Workstream A) writes the resolved `contextWindow` into `providers..contextWindow` so it survives across sessions without re-probing. Config schema gains the field. +- **Budget math.** `context-manager.ts` already reserves `min(4096, window/2)` for output and fires compaction at overflow — with a truthful window this becomes correct for free. `trimForContext` in each provider (currently `0.75 × window` heuristic) is replaced by an explicit `input ≤ window − outputReserve` expression via the manager. + +### 3.3 Tests required + +- Ladder resolution: num_ctx beats context_length; modelfile beats model_info; unknown → fallback. +- Systems with a *served* window smaller than *advertised* never produce a budget above served. +- Discovery repopulates `contextWindow` on models that were previously mis-typed `8192`. +- YAML round-trip preserves a persisted `contextWindow`. + +--- + +## 4. Design: Workstream C — Decode Parameter Profiles + +### 4.1 Default profile per task class + +The smart router already classifies models (`coding` / `instruct` / `general` / `embedding`) and routes per task (autocomplete / chat / agent). Attach a decode profile to (task × model-class): + +| task | temperature | top_p | top_k | seed | notes | +|---|---|---|---|---|---| +| tool-call / agent turn | 0.1–0.2 | 0.90 | – | fixed | deterministic tool use | +| code generation | 0.2–0.4 | 0.95 | low | – | lower repeat penalty | +| chat / explain | 0.70 | 0.90 | – | – | today's default | +| autocomplete (FIM) | 0.20 | 0.90 | – | fixed | determinism, low TTFT | + +### 4.2 Type + plumbing changes + +``` +ChatOptions (types.ts) + topK, minP, repeatPenalty, presencePenalty, + frequencyPenalty, seed, stop, numPredict +CompleteOptions + topP, topK, seed, presencePenalty, + frequencyPenalty, stop +LLMProviderConfig + decode profile object (profileId | partial overrides) +``` + +Backend field mapping (each provider maps `ChatOptions` → its native body): + +| option | backend A (native) | OpenAI-compatible | /props family | +|---|---|---|---| +| temperature | native `options.temperature` | `temperature` | `temperature` | +| top_p | native `options.top_p` | `top_p` | `top_p` | +| top_k | native `options.top_k` | `top_k` | `top_k` | +| min_p | native `options.min_p` | (v7 payload) `min_p` | (server payload) | +| repeat_penalty | native `options.repeat_penalty` | – | `repeat_penalty` | +| presence/frequency | – | `presence_penalty` / `frequency_penalty` | `presence_penalty` / `frequency_penalty` | +| seed | native `options.seed` | `seed` | `seed` | +| max tokens | native `options.num_predict` | `max_tokens` | `n_predict` | +| stop | native `options.stop` | `stop` | `stop` | + +Priority for a request's effective params: **explicit per-request override > task-profile default > config-provider `options` block > built-in default**. + +### 4.3 YAML surface + +```yaml +providers: + llama-compatible-server: + baseUrl: http://192.168.1.40:8000/v1 + model: code-model-7b + options: + temperature: 0.25 + topP: 0.95 + seed: 42 +``` + +Overrides win over the task profile. Schema validation added for the `options.*` block (types + bounds) so a typo surfaces at load time, not at request time. + +### 4.4 Tests required + +- Profile resolver: task × class → expected params; explicit override wins. +- Provider field mapping: each backend receives the mapped names (spy on request bodies via mocked fetch). +- Config schema rejects out-of-range `options.*` (e.g. `temperature > 2`). + +--- + +## 5. Design: Workstream D — Prefill & Structured Output + +### 5.1 Assistant-prefill for tool calls + +Today the prompt-based tool path detects *narrated-but-never-executed* tool calls after the fact and forces a `tool_call` re-emit. Prefill fixes the cause: seed the assistant turn's first tokens so the model starts generating inside the tool-call format. + +``` +Before (prompt-only, reactive) + user: find all TODOs in src/ + assistant: Sure! Let me look at that. First I will search… ← narration, + [no tool_call] then guard and retry + +After (assistant-prefill, pre-emptive) + user: find all TODOs in src/ + assistant: ← starts correct + tool_call{name:"search_files", args:{...}} first token +``` + +Design: at the start of an agent orchestration turn, if the backend uses prompt-based tools, append the assistant-prefill token(s) as the *first* assistant message content of the new turn (then stream the rest). Cost: deterministic tool calling, lower TTFB on the follow-up, no wasted narration turn. For OpenAI-compatible backends that support `continue_final_message`-style semantics this can be achieved by seeding the assistant prefix in the request. + +### 5.2 Stable prefix for KV/prompt-cache reuse + +Local backends auto-cache (or, when enabled, cache) the KV state of a repeated prefix. Multi-turn agent loops hit that cache only if the prefix is **byte-identical** across turns. Changes: + +1. Freeze assembly order: `system` → static tool definitions (constant serialization, never randomized JSON key order) → conversation. +2. Never re-serialize the same system/tool block differently between turns. +3. Explicitly toggle cache reuse where the backend exposes it (send the cache/CUDA-graph-friendly flag on the first request; rely on default for subsequent). + +Measurable target: turn N of a 10-turn loop re-prefills only the delta (system + tools serve from cache after turn 1). + +### 5.3 Structured output (grammar / JSON-schema-constrained generation) + +For backends with native structured-output support, force the tool-call result to match a schema *while generating*, making malformed calls impossible: + +| backend | mechanism | +|---|---| +| native (local inference) | per-call `format: json` + grammar or JSON-schema constrained decode | +| OpenAI-compatible | `response_format: {type: "json_object"}` / guided JSON (server feature) | +| /props family | JSON-schema grammar; `cache_prompt` on | + +Wire the tool-call schema (already available in `ToolParameterSchema`) into the constrained-decode path when the active backend advertises support. Fall back to the existing XML prompt parser otherwise (unchanged). + +### 5.4 Tests required + +- Prefill: agent turn emits the tool prefix as the first assistant content; no narration precedes it. +- Prefix stability: two turns with identical system+tools produce byte-identical assembled prompt prefixes. +- Structured output: constrained path sends the correct `format`/`response_format`; unmocked fallback still parses XML tools. +- KV-cache toggle: correct flag on first request, absent thereafter where required. + +--- + +## 6. End-to-end data flow (post-change) + +``` + New user + │ first-run + ▼ + [onboarding wizard] ──────────────────────────────────────────────┐ + │ provider │ endpoint │ key │ auto-detect models │ scan models │ + ▼ │ + device host servers ── probe/ladder ──▶ model metadata ────────┼─▶ persisted + (local endpoints, │ .champ/config.yaml + cloud APIs) │ { provider, providers.*, + │ runtime num_ctx ● model list ● capabilities │ options, contextWindow } + ▼ │ + [provider factory]──SecretStorage keys─────────────────────────┤ + │ │ + ▼ │ + [smart router] task decode profile + effective contextWindow │ + │ tool-call: temp 0.1, seed fixed │ + │ code: temp 0.3 │ + │ chat: temp 0.7 │ + ▼ │ + [agent orchestrator] context budget = window − outputReserve │ + │ trim / compact → fit budget ▲ │ + ▼ │ + [provider request] stable prefix + prefill + optional grammar │ + │ native options mapped per backend │ + ▼ │ + [backend] KV-cache reuse across turns · constrained decode │ + │ usage {input, output} → calibrated token estimator │ + ▼ │ + [tool registry] schema-validated execution → audit log │ +``` + +--- + +## 7. Compatibility & migration + +| Concern | Resolution | +|---|---| +| Existing `champ.*` settings users | One-time export-to-YAML notice (Workstream A) | +| Existing working `.champ/config.yaml` | Unchanged; wizard merges, never clobbers | +| Prompt-based XML tool fallback | Remains as the fallback under D; no behavioural regression | +| `champ.configSource = settings` semantics | Deleted with the settings layer | +| Provider `openai-compatible` for exotic endpoints | Unchanged, still the escape hatch for anything custom (incl. Apple-Silicon / AMD backends) | + +--- + +## 8. Non-goals + +- No new provider *types* (Apple-Silicon / AMD-specific engines are out of scope; they already work through the generic OpenAI-compatible adapter). +- No webview settings rewrite — the wizard is native QuickPick/InputBox. +- No change to the agent loop, approval flow, RAG indexing, or checkpoint behaviour except where context/decoding touches them. + +--- + +## 9. Rollout & validation + +1. **Per-ticket TDD**: failing unit tests written first, implementing to green. +2. **Per-ticket commit**: each workstream lands as one commit (independent of PR granularity). +3. **Gate**: `pnpm run check-types && pnpm run lint && pnpm run test:unit` (the merge gate). Integration/e2e suite run by the maintainer before merge. +4. **Manual smoke** (maintainer): wizard happy path, 401 key flow, offline-server fallback, existing config preserved. +5. **Release**: v1.8.0. + +--- + +## 10. Task breakdown → tickets + +| # | Ticket (title) | Workstream | Tests required (checklist inside ticket) | +|---|---|---|---| +| 1 | Guided provider setup wizard + YAML-only config | A | yaml-writer merge; wizard scope/provider/key/model decisions; settings-layer deletion; export-notice | +| 2 | Runtime context-window truth (served `num_ctx`) | B | ladder resolver; budget never exceeds served window; discovery repopulates; YAML persistence | +| 3 | Task-aware decode profiles + per-provider options | C | profile resolver; backend field mapping; options schema validation | +| 4 | Prefill & structured output (tool-call correctness) | D | assistant-prefill; prefix stability; constrained decode; KV-cache toggle | \ No newline at end of file From 00f79fe72c80b976982746889e90b6722e58caf4 Mon Sep 17 00:00:00 2001 From: hellodk Date: Fri, 28 Aug 2026 14:43:57 +0530 Subject: [PATCH 2/5] feat: serve the true context window from the runtime (num_ctx) instead of a hardcoded 8192 Adds a context-window resolver that prioritises the runtime num_ctx from /api/show parameters and modelfile over model_info defaults, caps the effective window with providers..contextWindow from YAML, and threads that cap through ProviderFactory into every provider config. Tests: num_ctx precedence, cap semantics, degenerate-value guards. Closes #119 --- src/config/config-loader.ts | 5 ++ src/config/context-window.ts | 56 ++++++++++++++ src/providers/factory.ts | 34 +++++++-- src/providers/ollama.ts | 29 ++++++-- src/providers/types.ts | 6 ++ test/unit/config/context-window.test.ts | 97 +++++++++++++++++++++++++ test/unit/providers/ollama.test.ts | 85 ++++++++++++++++++++++ 7 files changed, 299 insertions(+), 13 deletions(-) create mode 100644 src/config/context-window.ts create mode 100644 test/unit/config/context-window.test.ts diff --git a/src/config/config-loader.ts b/src/config/config-loader.ts index 81ed8b8..567bb6f 100644 --- a/src/config/config-loader.ts +++ b/src/config/config-loader.ts @@ -56,6 +56,11 @@ export interface ProviderConfig { apiKey?: string; /** Opt-in to native OpenAI tool calling (openai-compatible spec servers). */ supportsTools?: boolean; + /** + * Cap the effective context window for this provider (tokens). Never raise + * above what config pins, even if the runtime advertises more. Ticket #119. + */ + contextWindow?: number; } export interface AutocompleteConfig { diff --git a/src/config/context-window.ts b/src/config/context-window.ts new file mode 100644 index 0000000..15fff6b --- /dev/null +++ b/src/config/context-window.ts @@ -0,0 +1,56 @@ +/** + * Context-window resolution helpers. + * + * The effective context window for a model is the smallest of three numbers: + * - what the user pinned in config (never raise above this), + * - what the runtime declares (its served `num_ctx` / context length), + * - a sane floor so degenerate 0 values never become "no context". + * + * Ticket #119: stop trusting a hardcoded 8192. Prefer the value the server + * actually runs with, in priority order: + * 1. runtime `num_ctx` parameter (/api/show `parameters`) + * 2. runtime `num_ctx` in the modelfile (/api/show `modelfile`) + * 3. advertised context length in model metadata (model_info `*.context_length`) + * 4. fallback + */ +export interface ContextWindowMetadata { + /** Runtime num_ctx from /api/show `parameters` (overrides model_info). */ + numCtxParam?: number; + /** num_ctx baked into the modelfile (/api/show `modelfile`). */ + numCtxModelfile?: number; + /** Advertised context length in model metadata (model_info `*.context_length`). */ + modelInfoContextLength?: number; + /** User/admin ceiling from config — the window is never raised above this. */ + capWindow?: number; + /** Fallback when nothing is detected. */ + fallback: number; +} + +/** Parse a single `num_ctx ` line from a raw /api/show string. */ +export function parseNumCtxFromGrepLine(line: string): number | undefined { + const match = /^\s*num_ctx\s+(\d+)/.exec(line); + return match ? Number(match[1]) : undefined; +} + +/** Extract the runtime num_ctx from /api/show `parameters` or `modelfile`. */ +export function extractNumCtxFromParameters(raw?: string): number | undefined { + if (!raw) return undefined; + for (const line of raw.split("\n")) { + const parsed = parseNumCtxFromGrepLine(line); + if (parsed !== undefined) return parsed; + } + return undefined; +} + +export function resolveEffectiveContextWindow( + meta: ContextWindowMetadata, +): number { + const runtime = + meta.numCtxParam ?? meta.numCtxModelfile ?? meta.modelInfoContextLength; + + let effective = runtime && runtime > 0 ? runtime : meta.fallback; + if (meta.capWindow !== undefined && meta.capWindow > 0) { + effective = Math.min(effective, meta.capWindow); + } + return Math.max(1, effective); +} diff --git a/src/providers/factory.ts b/src/providers/factory.ts index 9cc6df7..062950d 100644 --- a/src/providers/factory.ts +++ b/src/providers/factory.ts @@ -115,25 +115,33 @@ export class ProviderFactory { switch (providerName) { case "claude": return new ClaudeProvider({ - ...this.baseConfig("claude"), + ...this.baseConfig("claude", { + contextWindow: providerEntry.contextWindow, + }), model: providerEntry.model ?? "claude-sonnet-4-20250514", apiKey: await getKey("champ.claude.apiKey", "ANTHROPIC_API_KEY"), }); case "openai": return new OpenAIProvider({ - ...this.baseConfig("openai"), + ...this.baseConfig("openai", { + contextWindow: providerEntry.contextWindow, + }), model: providerEntry.model ?? "gpt-4o", apiKey: await getKey("champ.openai.apiKey", "OPENAI_API_KEY"), }); case "gemini": return new GeminiProvider({ - ...this.baseConfig("gemini"), + ...this.baseConfig("gemini", { + contextWindow: providerEntry.contextWindow, + }), model: providerEntry.model ?? "gemini-2.0-flash", apiKey: await getKey("champ.gemini.apiKey", "GEMINI_API_KEY"), }); case "ollama": return new OllamaProvider({ - ...this.baseConfig("ollama"), + ...this.baseConfig("ollama", { + contextWindow: providerEntry.contextWindow, + }), model: providerEntry.model ?? "llama3.1", baseUrl: providerEntry.baseUrl ?? "http://localhost:11434", // apiKey from YAML (operator-issued) takes precedence over SecretStorage @@ -143,7 +151,9 @@ export class ProviderFactory { }); case "llamacpp": return new LlamaCppProvider({ - ...this.baseConfig("llamacpp"), + ...this.baseConfig("llamacpp", { + contextWindow: providerEntry.contextWindow, + }), model: providerEntry.model ?? "default", baseUrl: providerEntry.baseUrl ?? "http://localhost:8080/v1", apiKey: @@ -153,7 +163,9 @@ export class ProviderFactory { }); case "vllm": return new VLLMProvider({ - ...this.baseConfig("vllm"), + ...this.baseConfig("vllm", { + contextWindow: providerEntry.contextWindow, + }), model: providerEntry.model ?? "", baseUrl: providerEntry.baseUrl ?? "http://localhost:8000/v1", apiKey: @@ -163,7 +175,9 @@ export class ProviderFactory { }); case "openai-compatible": return new OpenAICompatibleProvider({ - ...this.baseConfig("openai-compatible"), + ...this.baseConfig("openai-compatible", { + contextWindow: providerEntry.contextWindow, + }), model: providerEntry.model ?? "default", baseUrl: providerEntry.baseUrl ?? "", apiKey: @@ -181,11 +195,15 @@ export class ProviderFactory { } } - private baseConfig(provider: string): Omit { + private baseConfig( + provider: string, + extras: Partial> = {}, + ): Omit { return { provider, maxTokens: 4096, temperature: 0.7, + ...extras, }; } diff --git a/src/providers/ollama.ts b/src/providers/ollama.ts index a001ec0..740e723 100644 --- a/src/providers/ollama.ts +++ b/src/providers/ollama.ts @@ -18,6 +18,10 @@ import type { ToolCall, } from "./types"; import { resilientFetch } from "./http-resilience"; +import { + extractNumCtxFromParameters, + resolveEffectiveContextWindow, +} from "@/config/context-window"; const DEFAULT_BASE_URL = "http://localhost:11434"; @@ -165,21 +169,33 @@ export class OllamaProvider implements LLMProvider { model_info?: Record; capabilities?: string[]; parameters?: string; + modelfile?: string; }; // Parse context window — key prefix varies: llama.context_length, qwen2.context_length, etc. + let modelInfoContextLength: number | undefined; if (data.model_info) { for (const [key, value] of Object.entries(data.model_info)) { if (key.endsWith(".context_length") && typeof value === "number") { - this.detectedContextWindow = value; - console.log( - `Champ: detected Ollama context window ${value} from /api/show (${key})`, - ); + modelInfoContextLength = value; break; } } } + // Runtime num_ctx (parameters/modelfile) overrides model_info: it is + // the window the server actually runs with (ticket #119). + this.detectedContextWindow = resolveEffectiveContextWindow({ + numCtxParam: extractNumCtxFromParameters(data.parameters), + numCtxModelfile: extractNumCtxFromParameters(data.modelfile), + modelInfoContextLength, + capWindow: this.config.contextWindow, + fallback: 8192, + }); + console.log( + `Champ: detected Ollama context window ${this.detectedContextWindow} from /api/show`, + ); + // Parse tool support from capabilities (Ollama v0.4+) if (Array.isArray(data.capabilities)) { this.detectedToolSupport = data.capabilities.includes("tools"); @@ -195,7 +211,10 @@ export class OllamaProvider implements LLMProvider { // Ensure context window is set (may be null if API didn't provide it) if (this.detectedContextWindow === null) { - this.detectedContextWindow = 8192; + this.detectedContextWindow = resolveEffectiveContextWindow({ + capWindow: this.config.contextWindow, + fallback: 8192, + }); } // detectedToolSupport stays null if API didn't report capabilities → fallback used } diff --git a/src/providers/types.ts b/src/providers/types.ts index 8f52a45..016f85c 100644 --- a/src/providers/types.ts +++ b/src/providers/types.ts @@ -146,6 +146,12 @@ export interface LLMProviderConfig { topP?: number; /** Additional headers to include on requests. */ customHeaders?: Record; + /** + * Cap the effective context window (tokens). When set, the provider never + * uses more than this window even if the runtime advertises a larger one + * (e.g. num_ctx override). Ticket #119. + */ + contextWindow?: number; /** Per-request HTTP timeout in ms before first token. Default: 120000 (issue #104). */ requestTimeoutMs?: number; /** diff --git a/test/unit/config/context-window.test.ts b/test/unit/config/context-window.test.ts new file mode 100644 index 0000000..d64e718 --- /dev/null +++ b/test/unit/config/context-window.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it } from "vitest"; +import { + extractNumCtxFromParameters, + parseNumCtxFromGrepLine, + resolveEffectiveContextWindow, +} from "@/config/context-window"; + +describe("parseNumCtxFromGrepLine (#119)", () => { + it("parses a raw modelfile num_ctx line", () => { + expect(parseNumCtxFromGrepLine("num_ctx 32768")).toBe(32768); + }); + + it("parses a parameters string line", () => { + expect(parseNumCtxFromGrepLine("num_ctx 16384")).toBe(16384); + }); + + it("parses a tool call in a single line", () => { + expect(parseNumCtxFromGrepLine('num_ctx 8192 "stop"')).toBe(8192); + }); + + it("returns undefined for unrelated content", () => { + expect(parseNumCtxFromGrepLine("temperature 0.7")).toBeUndefined(); + }); + + it("returns undefined for empty input", () => { + expect(parseNumCtxFromGrepLine("")).toBeUndefined(); + }); +}); + +describe("extractNumCtxFromParameters (#119)", () => { + it("extracts from a multi-line parameters string", () => { + const parameters = 'stop "<|start_header_id|>"\nnum_ctx 65536\nnum_gpu 99'; + expect(extractNumCtxFromParameters(parameters)).toBe(65536); + }); + + it("extracts from a single-line modelfile", () => { + expect(extractNumCtxFromParameters("FROM llama3.1\nnum_ctx 24576")).toBe( + 24576, + ); + }); + + it("returns undefined when num_ctx is absent", () => { + expect(extractNumCtxFromParameters("temperature 0.7")).toBeUndefined(); + }); +}); + +describe("resolveEffectiveContextWindow (#119)", () => { + it("prioritises the runtime num_ctx parameter over metadata", () => { + expect( + resolveEffectiveContextWindow({ + numCtxParam: 2048, + modelInfoContextLength: 8192, + fallback: 8192, + }), + ).toBe(2048); + }); + + it("falls back to model_info context_length when no num_ctx", () => { + expect( + resolveEffectiveContextWindow({ + modelInfoContextLength: 16384, + fallback: 8192, + }), + ).toBe(16384); + }); + + it("applies capWindow when present", () => { + expect( + resolveEffectiveContextWindow({ + numCtxParam: 65536, + modelInfoContextLength: 8192, + capWindow: 32768, + fallback: 8192, + }), + ).toBe(32768); + }); + + it("does not let capWindow raise the window", () => { + expect( + resolveEffectiveContextWindow({ + modelInfoContextLength: 4096, + capWindow: 16384, + fallback: 8192, + }), + ).toBe(4096); + }); + + it("uses the fallback when nothing is detected", () => { + expect(resolveEffectiveContextWindow({ fallback: 8192 })).toBe(8192); + }); + + it("returns a minimum of 1 for degenerate zero values", () => { + expect(resolveEffectiveContextWindow({ numCtxParam: 0, fallback: 1 })).toBe( + 1, + ); + }); +}); diff --git a/test/unit/providers/ollama.test.ts b/test/unit/providers/ollama.test.ts index 161279a..8837c42 100644 --- a/test/unit/providers/ollama.test.ts +++ b/test/unit/providers/ollama.test.ts @@ -433,3 +433,88 @@ describe("context window detection is awaited before trimForContext", () => { expect(chatBody.messages.length).toBeLessThan(50); }); }); + +describe("served context window truth (#119)", () => { + let provider: OllamaProvider; + + beforeEach(() => { + mockFetch.mockReset(); + provider = new OllamaProvider({ + provider: "ollama", + model: "small-model", + baseUrl: "http://localhost:11434", + maxTokens: 2048, + temperature: 0.7, + }); + }); + + const runChat = async () => { + const deltas: unknown[] = []; + for await (const d of provider.chat([{ role: "user", content: "hi" }])) { + deltas.push(d); + } + return deltas; + }; + + it("prefers the runtime num_ctx parameter over model_info context_length", async () => { + mockFetch + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ + model_info: { "llama.context_length": 8192 }, + parameters: 'stop "<|start_header_id|>"\nnum_ctx 2048', + modelfile: "FROM llama3.1\nnum_ctx 32768", + }), + }) + .mockResolvedValueOnce({ + ok: true, + body: createMockStream([ + { message: { content: "ok" }, done: false }, + { + message: { content: "" }, + done: true, + prompt_eval_count: 1, + eval_count: 1, + }, + ]), + }); + await runChat(); + // parameters num_ctx (2048) wins over modelfile (32768) and model_info (8192) + expect(provider.modelInfo().contextWindow).toBe(2048); + }); + + it("caps the effective window with the config contextWindow", async () => { + const capped = new OllamaProvider({ + provider: "ollama", + model: "small-model", + baseUrl: "http://localhost:11434", + maxTokens: 2048, + temperature: 0.7, + contextWindow: 4096, + }); + mockFetch + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ + model_info: { "llama.context_length": 65536 }, + }), + }) + .mockResolvedValueOnce({ + ok: true, + body: createMockStream([ + { message: { content: "ok" }, done: false }, + { + message: { content: "" }, + done: true, + prompt_eval_count: 1, + eval_count: 1, + }, + ]), + }); + const deltas: unknown[] = []; + for await (const d of capped.chat([{ role: "user", content: "hi" }])) { + deltas.push(d); + } + expect(capped.modelInfo().contextWindow).toBe(4096); + }); +}); From cd486b44a67e3205c6968dadd1eca879cccc83aa Mon Sep 17 00:00:00 2001 From: hellodk Date: Fri, 28 Aug 2026 14:54:11 +0530 Subject: [PATCH 3/5] feat: task-aware decode profiles + per-provider options block Adds per-task decode defaults (deterministic tool calls at 0.2, tighter coding turns, fixed autocomplete seed) with an effective-parameter merge: explicit request options win, then the YAML providers..options block, then the task profile, then built-in defaults. Backend field mapping: Ollama native options.* (temperature, top_k, min_p, repeat_penalty, seed, stop), OpenAI-compatible camelCase fields on the wire. Config schema now validates options.* types and bounds and rejects a non-integer contextWindow at load time. Tests: resolver precedence, per-backend request-body mapping, schema bounds. Closes #120 --- src/agent/agent-controller.ts | 1 + src/agent/tool-calling-loop.ts | 2 + src/completion/inline-provider.ts | 1 + src/config/config-loader.ts | 78 ++++++++ src/config/decode-profile.ts | 77 ++++++++ src/providers/factory.ts | 7 + src/providers/ollama.ts | 32 +++- src/providers/openai-compatible.ts | 44 ++++- src/providers/types.ts | 46 +++++ test/unit/config/config-loader.test.ts | 71 ++++++++ test/unit/config/decode-profile.test.ts | 104 +++++++++++ test/unit/providers/decode-mapping.test.ts | 201 +++++++++++++++++++++ 12 files changed, 654 insertions(+), 10 deletions(-) create mode 100644 src/config/decode-profile.ts create mode 100644 test/unit/config/decode-profile.test.ts create mode 100644 test/unit/providers/decode-mapping.test.ts diff --git a/src/agent/agent-controller.ts b/src/agent/agent-controller.ts index 9a9672b..d04c628 100644 --- a/src/agent/agent-controller.ts +++ b/src/agent/agent-controller.ts @@ -674,6 +674,7 @@ export class AgentController { // Native tool defs only when the provider says it supports them. tools: usePromptBased ? undefined : allTools, abortSignal: options.abortSignal, + taskHint: "coding", }); let errorOccurred = false; diff --git a/src/agent/tool-calling-loop.ts b/src/agent/tool-calling-loop.ts index 2dfd020..68dd0a9 100644 --- a/src/agent/tool-calling-loop.ts +++ b/src/agent/tool-calling-loop.ts @@ -131,6 +131,8 @@ export class ToolCallingLoop { const stream = this.provider.chat(history, { tools: toolDefs.length > 0 ? toolDefs : undefined, abortSignal: this.context.abortSignal, + // Deterministic decode profile for tool turns (ticket #120). + taskHint: "toolcall", }); for await (const delta of stream) { diff --git a/src/completion/inline-provider.ts b/src/completion/inline-provider.ts index a9515ec..f70f644 100644 --- a/src/completion/inline-provider.ts +++ b/src/completion/inline-provider.ts @@ -192,6 +192,7 @@ export class ChampInlineCompletionProvider { temperature: 0.2, maxTokens: 256, stop: DEFAULT_STOP, + taskHint: "completion", })) { if (controller.signal.aborted) return []; diff --git a/src/config/config-loader.ts b/src/config/config-loader.ts index 567bb6f..5d39069 100644 --- a/src/config/config-loader.ts +++ b/src/config/config-loader.ts @@ -61,6 +61,18 @@ export interface ProviderConfig { * above what config pins, even if the runtime advertises more. Ticket #119. */ contextWindow?: number; + /** Per-provider decode overrides (ticket #120). Overrides win over the task profile. */ + options?: { + temperature?: number; + topP?: number; + topK?: number; + minP?: number; + repeatPenalty?: number; + presencePenalty?: number; + frequencyPenalty?: number; + seed?: number; + stop?: string[]; + }; } export interface AutocompleteConfig { @@ -421,6 +433,72 @@ export class ConfigLoader { pc.supportsTools = c.supportsTools; } } + if ("contextWindow" in c) { + const cw = c as Record; + if ( + typeof cw.contextWindow !== "number" || + cw.contextWindow <= 0 || + !Number.isInteger(cw.contextWindow) + ) { + pushError( + `providers.${name}.contextWindow must be a positive integer`, + ); + } else { + pc.contextWindow = cw.contextWindow as number; + } + } + if ("options" in c) { + const opt = c as { options?: unknown }; + if (typeof opt.options !== "object" || opt.options === null) { + pushError(`providers.${name}.options must be an object`); + } else { + const o = opt.options as Record; + const out: NonNullable = {}; + const numField = ( + key: keyof typeof out, + min: number, + max: number, + hint?: string, + ): void => { + if (key in o) { + if ( + typeof o[key as string] !== "number" || + (o[key as string] as number) < min || + (o[key as string] as number) > max + ) { + pushError( + `providers.${name}.options.${String(key)} must be a number${ + hint ? ` ${hint}` : "" + }`, + ); + return; + } + out[key] = o[key as string] as never; + } + }; + numField("temperature", 0, 2); + numField("topP", 0, 1); + numField("topK", 0, Number.MAX_SAFE_INTEGER); + numField("minP", 0, 1); + numField("repeatPenalty", 0, Number.MAX_SAFE_INTEGER); + numField("presencePenalty", -2, 2); + numField("frequencyPenalty", -2, 2); + numField("seed", 0, Number.MAX_SAFE_INTEGER); + if ("stop" in o) { + if ( + !Array.isArray(o.stop) || + o.stop.some((s) => typeof s !== "string") + ) { + pushError( + `providers.${name}.options.stop must be an array of strings`, + ); + } else { + out.stop = o.stop as string[]; + } + } + pc.options = out; + } + } result.providers[name as ProviderName] = pc; } } diff --git a/src/config/decode-profile.ts b/src/config/decode-profile.ts new file mode 100644 index 0000000..1213aee --- /dev/null +++ b/src/config/decode-profile.ts @@ -0,0 +1,77 @@ +/** + * Task-aware decode profiles (ticket #120). + * + * Today every request runs at a fixed temperature 0.7. These profiles pick + * sane defaults per task class, so tool calls are deterministic and code + * generation runs tighter than creative chat. + * + * Effective-parameter precedence for a request (highest first): + * 1. explicit per-request options (passed to chat()/complete()) + * 2. the provider's YAML `options:` block (config-provider overrides) + * 3. the task decode profile (this module) + * 4. built-in provider config defaults (LLMProviderConfig fields) + * + * Note: this deliberately swaps tiers 2 and 3 relative to the first draft + * of the plan — a user who pins a value in YAML expects it to hold, so their + * overrides beat the automatic task profile. + */ +import type { DecodeParams, TaskHint } from "../providers/types"; + +export type DecodeTask = TaskHint; + +export const DEFAULT_PROFILES: Record = { + toolcall: { temperature: 0.2, topP: 0.9, seed: 42 }, + coding: { temperature: 0.3, topP: 0.95, topK: 40, repeatPenalty: 1.1 }, + chat: { temperature: 0.7, topP: 0.9 }, + completion: { temperature: 0.2, topP: 0.9, seed: 42 }, + embedding: {}, +}; + +const DECODE_KEYS: (keyof DecodeParams)[] = [ + "temperature", + "topP", + "topK", + "minP", + "repeatPenalty", + "presencePenalty", + "frequencyPenalty", + "seed", + "stop", +]; + +/** The profile for a task class, or an empty set when no hint is given. */ +export function resolveProfile(taskHint?: DecodeTask): DecodeParams { + return taskHint ? DEFAULT_PROFILES[taskHint] : {}; +} + +export interface MergeDecodeInput { + /** Options passed directly to the request (highest priority). */ + explicit?: DecodeParams; + /** The provider's YAML `options:` block (user overrides). */ + configOptions?: DecodeParams; + /** Task class whose profile fills gaps left by the upper layers. */ + taskHint?: DecodeTask; + /** Built-in provider config defaults (lowest priority, legacy settings). */ + base?: DecodeParams; +} + +/** + * Merge the layers into the effective decode parameters for a request. + * Later layers fill only the fields the higher-priority layers left + * undefined. + */ +export function mergeEffectiveDecode({ + explicit = {}, + configOptions = {}, + taskHint, + base = {}, +}: MergeDecodeInput): DecodeParams { + const profile = resolveProfile(taskHint); + const result: DecodeParams = {}; + for (const key of DECODE_KEYS) { + const value = + explicit[key] ?? configOptions[key] ?? profile[key] ?? base[key]; + if (value !== undefined) (result as Record)[key] = value; + } + return result; +} diff --git a/src/providers/factory.ts b/src/providers/factory.ts index 062950d..38b9266 100644 --- a/src/providers/factory.ts +++ b/src/providers/factory.ts @@ -117,6 +117,7 @@ export class ProviderFactory { return new ClaudeProvider({ ...this.baseConfig("claude", { contextWindow: providerEntry.contextWindow, + options: providerEntry.options, }), model: providerEntry.model ?? "claude-sonnet-4-20250514", apiKey: await getKey("champ.claude.apiKey", "ANTHROPIC_API_KEY"), @@ -125,6 +126,7 @@ export class ProviderFactory { return new OpenAIProvider({ ...this.baseConfig("openai", { contextWindow: providerEntry.contextWindow, + options: providerEntry.options, }), model: providerEntry.model ?? "gpt-4o", apiKey: await getKey("champ.openai.apiKey", "OPENAI_API_KEY"), @@ -133,6 +135,7 @@ export class ProviderFactory { return new GeminiProvider({ ...this.baseConfig("gemini", { contextWindow: providerEntry.contextWindow, + options: providerEntry.options, }), model: providerEntry.model ?? "gemini-2.0-flash", apiKey: await getKey("champ.gemini.apiKey", "GEMINI_API_KEY"), @@ -141,6 +144,7 @@ export class ProviderFactory { return new OllamaProvider({ ...this.baseConfig("ollama", { contextWindow: providerEntry.contextWindow, + options: providerEntry.options, }), model: providerEntry.model ?? "llama3.1", baseUrl: providerEntry.baseUrl ?? "http://localhost:11434", @@ -153,6 +157,7 @@ export class ProviderFactory { return new LlamaCppProvider({ ...this.baseConfig("llamacpp", { contextWindow: providerEntry.contextWindow, + options: providerEntry.options, }), model: providerEntry.model ?? "default", baseUrl: providerEntry.baseUrl ?? "http://localhost:8080/v1", @@ -165,6 +170,7 @@ export class ProviderFactory { return new VLLMProvider({ ...this.baseConfig("vllm", { contextWindow: providerEntry.contextWindow, + options: providerEntry.options, }), model: providerEntry.model ?? "", baseUrl: providerEntry.baseUrl ?? "http://localhost:8000/v1", @@ -177,6 +183,7 @@ export class ProviderFactory { return new OpenAICompatibleProvider({ ...this.baseConfig("openai-compatible", { contextWindow: providerEntry.contextWindow, + options: providerEntry.options, }), model: providerEntry.model ?? "default", baseUrl: providerEntry.baseUrl ?? "", diff --git a/src/providers/ollama.ts b/src/providers/ollama.ts index 740e723..791fffb 100644 --- a/src/providers/ollama.ts +++ b/src/providers/ollama.ts @@ -22,6 +22,7 @@ import { extractNumCtxFromParameters, resolveEffectiveContextWindow, } from "@/config/context-window"; +import { mergeEffectiveDecode } from "@/config/decode-profile"; const DEFAULT_BASE_URL = "http://localhost:11434"; @@ -240,13 +241,26 @@ export class OllamaProvider implements LLMProvider { this.inThinkingBlock = false; const url = `${this.config.baseUrl}/api/chat`; + const dec = mergeEffectiveDecode({ + explicit: options, + configOptions: this.config.options, + taskHint: options?.taskHint, + base: { temperature: this.config.temperature }, + }); const body = { model: this.config.model, messages: this.convertMessages(messages), stream: true, options: { - temperature: options?.temperature ?? this.config.temperature, - top_p: options?.topP ?? this.config.topP, + ...(dec.temperature !== undefined && { temperature: dec.temperature }), + ...(dec.topP !== undefined && { top_p: dec.topP }), + ...(dec.topK !== undefined && { top_k: dec.topK }), + ...(dec.minP !== undefined && { min_p: dec.minP }), + ...(dec.repeatPenalty !== undefined && { + repeat_penalty: dec.repeatPenalty, + }), + ...(dec.seed !== undefined && { seed: dec.seed }), + ...(dec.stop !== undefined && { stop: dec.stop }), num_predict: options?.maxTokens ?? this.config.maxTokens, }, tools: options?.tools?.map((t) => ({ @@ -312,14 +326,24 @@ export class OllamaProvider implements LLMProvider { } const url = `${this.config.baseUrl}/api/generate`; + const dec = mergeEffectiveDecode({ + explicit: options, + configOptions: this.config.options, + taskHint: options?.taskHint, + base: { temperature: this.config.temperature }, + }); const body = { model: this.config.model, prompt, stream: true, options: { - temperature: options?.temperature ?? this.config.temperature, + ...(dec.temperature !== undefined && { temperature: dec.temperature }), + ...(dec.topP !== undefined && { top_p: dec.topP }), + ...(dec.topK !== undefined && { top_k: dec.topK }), + ...(dec.minP !== undefined && { min_p: dec.minP }), + ...(dec.seed !== undefined && { seed: dec.seed }), + ...(dec.stop !== undefined && { stop: dec.stop }), num_predict: options?.maxTokens ?? this.config.maxTokens, - stop: options?.stop, }, }; diff --git a/src/providers/openai-compatible.ts b/src/providers/openai-compatible.ts index c00f799..8178fd4 100644 --- a/src/providers/openai-compatible.ts +++ b/src/providers/openai-compatible.ts @@ -19,6 +19,7 @@ import type { ContentBlock, } from "./types"; import { resilientFetch } from "./http-resilience"; +import { mergeEffectiveDecode } from "@/config/decode-profile"; const DEFAULT_CONTEXT_WINDOW = 8192; @@ -138,13 +139,29 @@ export class OpenAICompatibleProvider implements LLMProvider { } const url = this.joinUrl("/chat/completions"); - const body = { + const dec = mergeEffectiveDecode({ + explicit: options, + configOptions: this.config.options, + taskHint: options?.taskHint, + base: { temperature: this.config.temperature }, + }); + const body: Record = { model: this.config.model, messages: this.convertMessages(messages), stream: true, stream_options: { include_usage: true }, - temperature: options?.temperature ?? this.config.temperature, - top_p: options?.topP ?? this.config.topP, + ...(dec.temperature !== undefined && { temperature: dec.temperature }), + ...(dec.topP !== undefined && { top_p: dec.topP }), + ...(dec.topK !== undefined && { top_k: dec.topK }), + ...(dec.minP !== undefined && { min_p: dec.minP }), + ...(dec.presencePenalty !== undefined && { + presence_penalty: dec.presencePenalty, + }), + ...(dec.frequencyPenalty !== undefined && { + frequency_penalty: dec.frequencyPenalty, + }), + ...(dec.seed !== undefined && { seed: dec.seed }), + ...(dec.stop !== undefined && { stop: dec.stop }), max_tokens: options?.maxTokens ?? this.config.maxTokens, tools: options?.tools?.map((t) => ({ type: "function", @@ -239,13 +256,28 @@ export class OpenAICompatibleProvider implements LLMProvider { // sends the prompt verbatim, which is what FIM-aware completion // models expect. const url = this.joinUrl("/completions"); - const body = { + const dec = mergeEffectiveDecode({ + explicit: options, + configOptions: this.config.options, + taskHint: options?.taskHint, + base: { temperature: this.config.temperature }, + }); + const body: Record = { model: this.config.model, prompt, stream: true, - temperature: options?.temperature ?? this.config.temperature, + ...(dec.temperature !== undefined && { temperature: dec.temperature }), + ...(dec.topP !== undefined && { top_p: dec.topP }), + ...(dec.topK !== undefined && { top_k: dec.topK }), + ...(dec.seed !== undefined && { seed: dec.seed }), + ...(dec.presencePenalty !== undefined && { + presence_penalty: dec.presencePenalty, + }), + ...(dec.frequencyPenalty !== undefined && { + frequency_penalty: dec.frequencyPenalty, + }), + ...(dec.stop !== undefined && { stop: dec.stop }), max_tokens: options?.maxTokens ?? this.config.maxTokens, - stop: options?.stop, }; try { diff --git a/src/providers/types.ts b/src/providers/types.ts index 016f85c..41667ca 100644 --- a/src/providers/types.ts +++ b/src/providers/types.ts @@ -152,6 +152,12 @@ export interface LLMProviderConfig { * (e.g. num_ctx override). Ticket #119. */ contextWindow?: number; + /** + * Per-provider decode overrides from the YAML `options:` block (ticket + * #120). Explicit per-request options win over these; these win over the + * task decode profile. + */ + options?: DecodeParams; /** Per-request HTTP timeout in ms before first token. Default: 120000 (issue #104). */ requestTimeoutMs?: number; /** @@ -163,6 +169,30 @@ export interface LLMProviderConfig { supportsTools?: boolean; } +/** + * Decode parameters shared by chat and completion requests (ticket #120). + * Each provider maps these onto its native body field names. + */ +export interface DecodeParams { + temperature?: number; + topP?: number; + topK?: number; + minP?: number; + repeatPenalty?: number; + presencePenalty?: number; + frequencyPenalty?: number; + seed?: number; + stop?: string[]; +} + +/** Task class used to pick the default decode profile (ticket #120). */ +export type TaskHint = + | "coding" + | "chat" + | "completion" + | "toolcall" + | "embedding"; + /** * Runtime options for a single chat request. */ @@ -172,6 +202,15 @@ export interface ChatOptions { temperature?: number; maxTokens?: number; topP?: number; + topK?: number; + minP?: number; + repeatPenalty?: number; + presencePenalty?: number; + frequencyPenalty?: number; + seed?: number; + stop?: string[]; + /** Task class for picking the default decode profile (ticket #120). */ + taskHint?: TaskHint; /** When true, instruct the provider to output valid JSON only. Supported by Ollama and some OpenAI-compatible providers. */ jsonFormat?: boolean; } @@ -183,7 +222,14 @@ export interface CompleteOptions { abortSignal?: AbortSignal; temperature?: number; maxTokens?: number; + topP?: number; + topK?: number; + seed?: number; + presencePenalty?: number; + frequencyPenalty?: number; stop?: string[]; + /** Task class for picking the default decode profile (ticket #120). */ + taskHint?: TaskHint; } /** diff --git a/test/unit/config/config-loader.test.ts b/test/unit/config/config-loader.test.ts index 3e4e5d6..6169e7f 100644 --- a/test/unit/config/config-loader.test.ts +++ b/test/unit/config/config-loader.test.ts @@ -124,6 +124,77 @@ providers: `; expect(() => ConfigLoader.parseYaml(yaml)).toThrow(/apikey/i); }); + + it("rejects options.temperature out of range (#120)", () => { + const yaml = ` +provider: ollama +providers: + ollama: + model: llama3.1 + options: + temperature: 2.5 +`; + expect(() => ConfigLoader.parseYaml(yaml)).toThrow(/temperature/i); + }); + + it("rejects options.topP out of range (#120)", () => { + const yaml = ` +provider: ollama +providers: + ollama: + options: + topP: 1.5 +`; + expect(() => ConfigLoader.parseYaml(yaml)).toThrow(/topp/i); + }); + + it("rejects a non-string stop entry (#120)", () => { + const yaml = ` +provider: ollama +providers: + ollama: + options: + stop: [END, 42] +`; + expect(() => ConfigLoader.parseYaml(yaml)).toThrow(/stop/i); + }); + + it("accepts a valid options block (#120)", () => { + const yaml = ` +provider: ollama +providers: + ollama: + options: + temperature: 0.25 + topP: 0.95 + seed: 42 + stop: [""] +`; + const config = ConfigLoader.parseYaml(yaml); + expect(config.providers?.ollama?.options?.temperature).toBe(0.25); + expect(config.providers?.ollama?.options?.seed).toBe(42); + }); + + it("accepts contextWindow as a positive integer (#119)", () => { + const yaml = ` +provider: ollama +providers: + ollama: + contextWindow: 16384 +`; + const config = ConfigLoader.parseYaml(yaml); + expect(config.providers?.ollama?.contextWindow).toBe(16384); + }); + + it("rejects a non-integer contextWindow (#119)", () => { + const yaml = ` +provider: ollama +providers: + ollama: + contextWindow: 16.5 +`; + expect(() => ConfigLoader.parseYaml(yaml)).toThrow(/contextwindow/i); + }); }); describe("merge", () => { diff --git a/test/unit/config/decode-profile.test.ts b/test/unit/config/decode-profile.test.ts new file mode 100644 index 0000000..9fd52bf --- /dev/null +++ b/test/unit/config/decode-profile.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it } from "vitest"; +import { + DEFAULT_PROFILES, + mergeEffectiveDecode, + resolveProfile, + type DecodeTask, +} from "@/config/decode-profile"; + +describe("default decode profiles (#120)", () => { + it("every task class has a profile", () => { + const tasks: DecodeTask[] = [ + "coding", + "chat", + "completion", + "toolcall", + "embedding", + ]; + for (const t of tasks) { + expect(resolveProfile(t)).toBeDefined(); + } + }); + + it("toolcall is deterministic (low temperature, fixed seed)", () => { + const p = resolveProfile("toolcall"); + expect(p.temperature).toBeLessThanOrEqual(0.2); + expect(p.seed).toBeDefined(); + }); + + it("autocomplete (completion) is deterministic", () => { + const p = resolveProfile("completion"); + expect(p.temperature).toBeLessThanOrEqual(0.3); + }); + + it("chat keeps the historical 0.7 default", () => { + expect(resolveProfile("chat").temperature).toBe(0.7); + }); + + it("coding is tighter than chat with a top-p of 0.95", () => { + const p = resolveProfile("coding"); + expect(p.temperature).toBeLessThan(0.7); + expect(p.topP).toBe(0.95); + }); + + it("embedding has no decode parameters", () => { + expect(Object.keys(resolveProfile("embedding")).length).toBe(0); + }); +}); + +describe("mergeEffectiveDecode precedence (#120)", () => { + it("explicit per-request override beats config options and profile", () => { + const r = mergeEffectiveDecode({ + explicit: { temperature: 0.1 }, + configOptions: { temperature: 0.9, topP: 0.5 }, + taskHint: "toolcall", + }); + expect(r.temperature).toBe(0.1); + }); + + it("config options win over the task profile", () => { + const r = mergeEffectiveDecode({ + configOptions: { temperature: 0.9 }, + taskHint: "toolcall", + }); + expect(r.temperature).toBe(0.9); + }); + + it("the task profile fills fields the user did not pin", () => { + const r = mergeEffectiveDecode({ + configOptions: { topP: 0.5 }, + taskHint: "toolcall", + }); + expect(r.temperature).toBe(DEFAULT_PROFILES.toolcall.temperature); + expect(r.seed).toBe(DEFAULT_PROFILES.toolcall.seed); + expect(r.topP).toBe(0.5); + }); + + it("no hint and no config keeps the built-in temperature when supplied", () => { + const r = mergeEffectiveDecode({ + configOptions: { temperature: 0.7 }, + }); + expect(r.temperature).toBe(0.7); + }); + + it("no layers at all yields an empty set (server defaults)", () => { + expect(Object.keys(mergeEffectiveDecode({})).length).toBe(0); + }); + + it("zero is honoured as an explicit value", () => { + const r = mergeEffectiveDecode({ + explicit: { temperature: 0 }, + configOptions: { temperature: 0.9 }, + taskHint: "coding", + }); + expect(r.temperature).toBe(0); + }); + + it("stop arrays flow through the layers", () => { + const r = mergeEffectiveDecode({ + explicit: { stop: ["END"] }, + taskHint: "chat", + }); + expect(r.stop).toEqual(["END"]); + }); +}); diff --git a/test/unit/providers/decode-mapping.test.ts b/test/unit/providers/decode-mapping.test.ts new file mode 100644 index 0000000..c9d8ea9 --- /dev/null +++ b/test/unit/providers/decode-mapping.test.ts @@ -0,0 +1,201 @@ +/** + * TDD: per-backend decode-parameter field mapping (ticket #120). + * Each provider maps effective ChatOptions onto its native body names. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { OllamaProvider } from "@/providers/ollama"; +import { OpenAICompatibleProvider } from "@/providers/openai-compatible"; +import { DEFAULT_PROFILES } from "@/config/decode-profile"; + +function createMockStream(chunks: object[]): ReadableStream { + return new ReadableStream({ + start(controller) { + for (const chunk of chunks) { + controller.enqueue( + new TextEncoder().encode(JSON.stringify(chunk) + "\n"), + ); + } + controller.close(); + }, + }); +} + +function sseBody(chunks: object[]): ReadableStream { + const encoder = new TextEncoder(); + const parts = chunks + .map((c) => `data: ${JSON.stringify(c)}\n\n`) + .concat(["data: [DONE]\n\n"]); + return new ReadableStream({ + start(controller) { + for (const part of parts) controller.enqueue(encoder.encode(part)); + controller.close(); + }, + }); +} + +describe("Ollama native body mapping (#120)", () => { + const mockFetch = vi.fn(); + + beforeEach(() => { + mockFetch.mockReset(); + }); + + it("maps effective params onto native options.* names", async () => { + let captured: Record = {}; + mockFetch + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ model_info: {} }), + }) + .mockImplementationOnce(async (_url, init) => { + captured = JSON.parse(init?.body as string); + return { + ok: true, + body: createMockStream([{ message: { content: "hi" }, done: true }]), + } as unknown as Response; + }); + vi.stubGlobal("fetch", mockFetch); + + const provider = new OllamaProvider({ + provider: "ollama", + model: "m", + baseUrl: "http://localhost:11434", + maxTokens: 2048, + temperature: 0.7, + }); + for await (const _ of provider.chat([{ role: "user", content: "hi" }], { + taskHint: "toolcall", + })) { + // drain + } + const opts = captured.options as Record; + expect(opts.temperature).toBe(DEFAULT_PROFILES.toolcall.temperature); + expect(opts.top_p).toBe(DEFAULT_PROFILES.toolcall.topP); + expect(opts.seed).toBe(DEFAULT_PROFILES.toolcall.seed); + expect(opts.num_predict).toBe(2048); + }); + + it("explicit temperature overrides the profile and YAML options", async () => { + let captured: Record = {}; + mockFetch + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ model_info: {} }), + }) + .mockImplementationOnce(async (_url, init) => { + captured = JSON.parse(init?.body as string); + return { + ok: true, + body: createMockStream([{ message: { content: "hi" }, done: true }]), + } as unknown as Response; + }); + vi.stubGlobal("fetch", mockFetch); + + const provider = new OllamaProvider({ + provider: "ollama", + model: "m", + baseUrl: "http://localhost:11434", + maxTokens: 2048, + temperature: 0.7, + options: { temperature: 0.9 }, + }); + for await (const _ of provider.chat([{ role: "user", content: "hi" }], { + taskHint: "toolcall", + temperature: 0.1, + })) { + // drain + } + const opts = captured.options as Record; + expect(opts.temperature).toBe(0.1); + }); + + it("YAML options beat the task profile", async () => { + let captured: Record = {}; + mockFetch + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ model_info: {} }), + }) + .mockImplementationOnce(async (_url, init) => { + captured = JSON.parse(init?.body as string); + return { + ok: true, + body: createMockStream([{ message: { content: "hi" }, done: true }]), + } as unknown as Response; + }); + vi.stubGlobal("fetch", mockFetch); + + const provider = new OllamaProvider({ + provider: "ollama", + model: "m", + baseUrl: "http://localhost:11434", + maxTokens: 2048, + temperature: 0.7, + options: { temperature: 0.9, topK: 99 }, + }); + for await (const _ of provider.chat([{ role: "user", content: "hi" }], { + taskHint: "toolcall", + })) { + // drain + } + const opts = captured.options as Record; + expect(opts.temperature).toBe(0.9); + expect(opts.top_k).toBe(99); + }); +}); + +describe("OpenAI-compatible body mapping (#120)", () => { + it("maps effective params onto OpenAI field names", async () => { + let captured: Record = {}; + vi.spyOn(global, "fetch").mockImplementation(async (_url, init) => { + captured = JSON.parse(init?.body as string); + return { + ok: true, + body: sseBody([{ choices: [{ delta: {} }] }]), + } as unknown as Response; + }); + + const provider = new OpenAICompatibleProvider({ + provider: "openai-compatible", + model: "m", + baseUrl: "http://localhost:11434", + maxTokens: 4096, + }); + for await (const _ of provider.chat([{ role: "user", content: "hi" }], { + taskHint: "toolcall", + })) { + // drain + } + expect(captured.temperature).toBe(DEFAULT_PROFILES.toolcall.temperature); + expect(captured.top_p).toBe(DEFAULT_PROFILES.toolcall.topP); + expect(captured.seed).toBe(DEFAULT_PROFILES.toolcall.seed); + expect(captured.max_tokens).toBe(4096); + }); + + it("explicit override wins over the profile", async () => { + let captured: Record = {}; + vi.spyOn(global, "fetch").mockImplementation(async (_url, init) => { + captured = JSON.parse(init?.body as string); + return { + ok: true, + body: sseBody([{ choices: [{ delta: {} }] }]), + } as unknown as Response; + }); + + const provider = new OpenAICompatibleProvider({ + provider: "openai-compatible", + model: "m", + baseUrl: "http://localhost:11434", + maxTokens: 4096, + }); + for await (const _ of provider.chat([{ role: "user", content: "hi" }], { + taskHint: "toolcall", + temperature: 0.05, + seed: 7, + })) { + // drain + } + expect(captured.temperature).toBe(0.05); + expect(captured.seed).toBe(7); + }); +}); From 896c0031baabbc91a93d45d1d5fff851761e08c0 Mon Sep 17 00:00:00 2001 From: hellodk Date: Fri, 28 Aug 2026 15:11:14 +0530 Subject: [PATCH 4/5] feat: guided provider setup wizard, YAML-only config source Closes #118 The configure-provider command walks provider -> endpoint -> (401?) API key capture -> live model discovery and merges the result into the effective .champ/config.yaml via the new merge-preserving writer, then hot-reloads the provider. Cloud providers with a stored key keep their current model; self-hosted providers only prompt for a key when the endpoint probe actually returns 401/403. configSource is gone: YAML is the single source of truth. The legacy workspace-settings layer, its champ.* flat keys, and the createFromConfig() factory path are deleted so there is nowhere left to fall back to. --- package.json | 75 +-------- src/config/config-loader.ts | 21 +-- src/config/setup-wizard.ts | 100 +++++++++++ src/config/yaml-writer.ts | 91 ++++++++++ src/extension.ts | 215 +++++++++++++++++++++--- src/providers/factory.ts | 159 +----------------- test/unit/config/layered-source.test.ts | 28 ++- test/unit/config/setup-wizard.test.ts | 123 ++++++++++++++ test/unit/config/yaml-writer.test.ts | 164 ++++++++++++++++++ test/unit/providers/factory.test.ts | 116 +------------ 10 files changed, 704 insertions(+), 388 deletions(-) create mode 100644 src/config/setup-wizard.ts create mode 100644 src/config/yaml-writer.ts create mode 100644 test/unit/config/setup-wizard.test.ts create mode 100644 test/unit/config/yaml-writer.test.ts diff --git a/package.json b/package.json index b108745..1594255 100644 --- a/package.json +++ b/package.json @@ -177,6 +177,11 @@ "command": "champ.resumeTeamRun", "title": "Champ: Resume Team Run" }, + { + "command": "champ.configureProvider", + "title": "Champ: Configure Provider", + "category": "Champ" + }, { "command": "champ.openTeamBuilder", "title": "Champ: Open Team Builder" @@ -252,81 +257,11 @@ "configuration": { "title": "Champ", "properties": { - "champ.provider": { - "type": "string", - "default": "claude", - "enum": [ - "claude", - "openai", - "gemini", - "ollama", - "llamacpp", - "vllm", - "openai-compatible" - ], - "markdownDescription": "Active LLM provider. Edit **`.champ/config.yaml`** for full control (YAML overrides these settings)." - }, - "champ.claude.model": { - "type": "string", - "default": "claude-sonnet-4-20250514", - "markdownDescription": "Claude model ID. API key is stored securely — run **Champ: Set API Key** to configure it." - }, - "champ.openai.model": { - "type": "string", - "default": "gpt-4o", - "markdownDescription": "OpenAI model ID. API key is stored securely — run **Champ: Set API Key** to configure it." - }, - "champ.gemini.model": { - "type": "string", - "default": "gemini-2.0-flash", - "markdownDescription": "Gemini model ID. API key is stored securely — run **Champ: Set API Key** to configure it." - }, - "champ.ollama.baseUrl": { - "type": "string", - "default": "http://localhost:11434", - "markdownDescription": "Ollama server base URL. Default is local; set to a remote address if Ollama runs on another machine." - }, - "champ.ollama.model": { - "type": "string", - "default": "", - "markdownDescription": "Ollama model tag. Must match an installed model — run `ollama pull ` to install." - }, - "champ.llamacpp.baseUrl": { - "type": "string", - "default": "http://localhost:8080/v1", - "markdownDescription": "llama.cpp server base URL **including `/v1`** (e.g. `http://localhost:8080/v1`). Set the model via `.champ/config.yaml`." - }, - "champ.vllm.baseUrl": { - "type": "string", - "default": "http://localhost:8000/v1", - "markdownDescription": "vLLM server base URL **including `/v1`** (e.g. `http://localhost:8000/v1`). Set the model via `.champ/config.yaml`." - }, - "champ.vllm.apiKey": { - "type": "string", - "markdownDescription": "vLLM API key (if your server requires authentication). Store securely via **Champ: Set API Key** command." - }, - "champ.openaiCompatible.baseUrl": { - "type": "string", - "markdownDescription": "Base URL for an OpenAI-compatible server **including `/v1`**." - }, - "champ.openaiCompatible.model": { - "type": "string" - }, - "champ.openaiCompatible.apiKey": { - "type": "string", - "markdownDescription": "API key for the OpenAI-compatible server. Store securely via **Champ: Set API Key** command." - }, "champ.yoloMode": { "type": "boolean", "default": false, "markdownDescription": "Skip approval prompts for terminal commands and file edits. **Use with caution.**" }, - "champ.configSource": { - "type": "string", - "enum": ["auto", "workspace-yaml", "user-yaml", "settings"], - "default": "auto", - "markdownDescription": "Single source of truth for Champ's LLM configuration.\n\n- **auto** — workspace `.champ/config.yaml` if present, else `~/.champ/config.yaml`, else these settings (historical precedence)\n- **workspace-yaml / user-yaml / settings** — use ONLY that source; others are ignored\n\nInspect the running result with **Champ: Show Effective Config**." - }, "champ.autoFix.enabled": { "type": "boolean", "default": true, diff --git a/src/config/config-loader.ts b/src/config/config-loader.ts index 5d39069..3837ee8 100644 --- a/src/config/config-loader.ts +++ b/src/config/config-loader.ts @@ -1419,13 +1419,9 @@ export class ConfigLoader { // Layered resolution with explicit single source (issue #115) // --------------------------------------------------------------------------- -export type ConfigSource = "auto" | "workspace-yaml" | "user-yaml" | "settings"; +export type ConfigSource = "auto" | "workspace-yaml" | "user-yaml"; -export type ConfigLayer = - | "workspace-yaml" - | "user-yaml" - | "settings" - | "default"; +export type ConfigLayer = "workspace-yaml" | "user-yaml" | "default"; export interface LayeredInput { /** Raw text of /.champ/config.yaml (null if absent). */ @@ -1476,12 +1472,7 @@ export function resolveLayered(input: LayeredInput): LayeredResult { let wsText = input.workspaceText ?? null; let userText = input.userText ?? null; - if (input.source === "settings") { - if (wsText) ignoredSources.push("workspace-yaml"); - if (userText) ignoredSources.push("user-yaml"); - wsText = null; - userText = null; - } else if (input.source === "workspace-yaml") { + if (input.source === "workspace-yaml") { if (userText) ignoredSources.push("user-yaml"); userText = null; } else if (input.source === "user-yaml") { @@ -1494,12 +1485,12 @@ export function resolveLayered(input: LayeredInput): LayeredResult { if (input.source === "workspace-yaml" && !wsText) { throw new Error( - "Invalid YAML or missing file: workspace .champ/config.yaml not found but champ.configSource=workspace-yaml", + "Invalid YAML or missing file: workspace .champ/config.yaml not found but source=workspace-yaml", ); } if (input.source === "user-yaml" && !userText) { throw new Error( - "Invalid YAML or missing file: ~/.champ/config.yaml not found but champ.configSource=user-yaml", + "Invalid YAML or missing file: ~/.champ/config.yaml not found but source=user-yaml", ); } @@ -1509,7 +1500,7 @@ export function resolveLayered(input: LayeredInput): LayeredResult { if (!ws && !user) { return { config: null, - usedSource: "settings", + usedSource: "default", ignoredSources, conflict: false, origins: {}, diff --git a/src/config/setup-wizard.ts b/src/config/setup-wizard.ts new file mode 100644 index 0000000..2241221 --- /dev/null +++ b/src/config/setup-wizard.ts @@ -0,0 +1,100 @@ +/** + * Guided provider setup wizard flow controller (ticket #118). + * + * Pure orchestration over injected dependencies so the decision logic is + * unit-testable without the VS Code UI. The command handler in extension.ts + * supplies real prompts, fetch, model discovery, and SecretStorage. + */ + +export interface WizardProviderOption { + id: string; + /** Cloud providers keep their configured model when a key is stored. */ + cloud: boolean; +} + +export interface WizardOutcomeCancelled { + status: "cancelled"; +} + +export interface WizardOutcomeOk { + status: "ok"; + providerId: string; + baseUrl: string; + /** undefined = keep the configured model. */ + model?: string; + /** Set when a fresh key was captured and stored. */ + apiKey?: string; + keyStored: boolean; +} + +export type WizardOutcome = WizardOutcomeCancelled | WizardOutcomeOk; + +export interface WizardDeps { + providers: WizardProviderOption[]; + pickProvider(options: WizardProviderOption[]): Promise; + inputBaseUrl(providerId: string): Promise; + probeEndpoint( + providerId: string, + baseUrl: string, + ): Promise<{ status: number } | { status: undefined; ok: boolean }>; + hasStoredApiKey(providerId: string): Promise; + promptForApiKey(providerId: string): Promise; + storeKey(providerId: string, apiKey: string): Promise; + fetchModels(providerId: string, baseUrl: string): Promise; + pickModel(models: string[]): Promise; +} + +export async function runWizard(deps: WizardDeps): Promise { + const providerId = await deps.pickProvider(deps.providers); + if (!providerId) return { status: "cancelled" }; + + const baseUrl = await deps.inputBaseUrl(providerId); + if (!baseUrl) return { status: "cancelled" }; + + const provider = deps.providers.find((p) => p.id === providerId); + const cloud = Boolean(provider?.cloud); + + const probe = await deps.probeEndpoint(providerId, baseUrl); + const probeStatus = + "status" in probe && typeof probe.status === "number" + ? probe.status + : probe.ok + ? 200 + : 0; + + const outcome: WizardOutcomeOk = { + status: "ok", + providerId, + baseUrl, + keyStored: false, + }; + + if (probeStatus === 401 || probeStatus === 403) { + const apiKey = await deps.promptForApiKey(providerId); + if (!apiKey) return { status: "cancelled" }; + await deps.storeKey(providerId, apiKey); + outcome.apiKey = apiKey; + outcome.keyStored = true; + } + + // Cloud + stored key → keep the configured model, skip discovery picking. + if (cloud) { + const hasKey = + outcome.keyStored || (await deps.hasStoredApiKey(providerId)); + if (hasKey) return outcome; + } + + const models = await deps.fetchModels(providerId, baseUrl); + if (models.length === 0) { + // No listable model — leave model untouched so the operator's yaml model + // (or the provider default) applies. + return outcome; + } + if (models.length === 1) { + outcome.model = models[0]; + return outcome; + } + const picked = await deps.pickModel(models); + if (picked) outcome.model = picked; + return outcome; +} diff --git a/src/config/yaml-writer.ts b/src/config/yaml-writer.ts new file mode 100644 index 0000000..4357d73 --- /dev/null +++ b/src/config/yaml-writer.ts @@ -0,0 +1,91 @@ +/** + * YAML merge-writer for guided setup (ticket #118). + * + * The wizard updates one provider block in .champ/config.yaml without + * touching the rest of the file. The file is parsed and re-dumped with + * js-yaml, so every other section and key is preserved verbatim; the only + * trade-off is that comments are dropped from the output. Safe enough for a + * wizard: the user seals the file after the run, and hand-edits after that + * are left alone because the writer only rewrites on wizard runs. + */ +import * as yaml from "js-yaml"; + +export interface ProviderYamlPatch { + /** Provider id, must match the key under `providers:` in config.yaml. */ + providerId: string; + baseUrl?: string; + model?: string; + contextWindow?: number; + options?: Record; +} + +export interface UpsertOptions { + /** Also set the top-level `provider:` to the patched provider. */ + setActive?: boolean; +} + +function cleanPatch(patch: ProviderYamlPatch): Record { + const out: Record = {}; + if (patch.baseUrl !== undefined) out.baseUrl = patch.baseUrl; + if (patch.model !== undefined) out.model = patch.model; + if (patch.contextWindow !== undefined) + out.contextWindow = patch.contextWindow; + if (patch.options !== undefined && Object.keys(patch.options).length > 0) { + out.options = patch.options; + } + return out; +} + +/** True when the parsed config already has a block for this provider. */ +export function providerExistsInYaml( + text: string | null | undefined, + providerId: string, +): boolean { + if (!text?.trim()) return false; + try { + const parsed = yaml.load(text) as Record | null; + if (!parsed || typeof parsed !== "object") return false; + const providers = parsed.providers as Record | undefined; + return Boolean(providers && providers[providerId]); + } catch { + return false; + } +} + +export function upsertProviderInYaml( + text: string | null | undefined, + patch: ProviderYamlPatch, + opts: UpsertOptions = {}, +): { yaml: string; created: boolean } { + let root: Record; + let created = false; + if (text?.trim()) { + try { + const parsed = yaml.load(text); + root = + parsed && typeof parsed === "object" + ? (parsed as Record) + : {}; + } catch { + // Untrusted/legacy content — start clean rather than clobber valid keys. + root = {}; + } + } else { + root = {}; + created = true; + } + + const providers = + (root.providers as Record | undefined) ?? {}; + const current = + (providers[patch.providerId] as Record | undefined) ?? {}; + providers[patch.providerId] = { + ...current, + ...cleanPatch(patch), + }; + root.providers = providers; + + if (opts.setActive) root.provider = patch.providerId; + + return { yaml: yaml.dump(root), created }; +} diff --git a/src/extension.ts b/src/extension.ts index 8b6f01a..87bc198 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -39,6 +39,9 @@ import { ChunkingService } from "./indexing/chunking-service"; import { RepoMapBuilder } from "./indexing/repo-map-builder"; import { ContextResolver } from "./agent/context-resolver"; import { buildProbeUrl } from "./utils/probe-url"; +import { resilientFetch } from "./providers/http-resilience"; +import { runWizard } from "./config/setup-wizard"; +import { upsertProviderInYaml } from "./config/yaml-writer"; import { ConfigLoader, resolveLayered, @@ -1591,6 +1594,175 @@ export async function activate( "Champ: created .champ/config.yaml. Edit it and save to apply.", ); }), + // Guided provider setup wizard (#118). Collects provider → endpoint → + // (401? API key) → live model discovery and merges the result into the + // effective YAML config, then hot-reloads the provider. + vscode.commands.registerCommand("champ.configureProvider", async () => { + const wizardProviders: { + id: import("./config/config-loader").ProviderName; + cloud: boolean; + }[] = [ + { id: "claude", cloud: true }, + { id: "openai", cloud: true }, + { id: "gemini", cloud: true }, + { id: "ollama", cloud: false }, + { id: "llamacpp", cloud: false }, + { id: "vllm", cloud: false }, + { id: "openai-compatible", cloud: false }, + ]; + + const SECRET_KEY: Record = { + claude: "champ.claude.apiKey", + openai: "champ.openai.apiKey", + gemini: "champ.gemini.apiKey", + vllm: "champ.vllm.apiKey", + "openai-compatible": "champ.openaiCompatible.apiKey", + }; + + const DEFAULT_BASE_URL: Record = { + claude: "https://api.anthropic.com/v1", + openai: "https://api.openai.com/v1", + gemini: "https://generativelanguage.googleapis.com/v1beta", + ollama: "http://localhost:11434", + llamacpp: "http://localhost:8080/v1", + vllm: "http://localhost:8000/v1", + }; + + const listModels = async ( + providerId: string, + baseUrl: string, + ): Promise => { + try { + if (providerId === "ollama") { + const res = await resilientFetch( + `${baseUrl.replace(/\/+$/, "")}/api/tags`, + {}, + { timeoutMs: 8000, maxRetries: 0 }, + ); + if (!res.ok) return []; + const data = (await res.json()) as { + models?: Array<{ model?: string; name?: string }>; + }; + return (data.models ?? []) + .map((m) => m.model ?? m.name ?? "") + .filter(Boolean); + } + const res = await resilientFetch( + buildProbeUrl(baseUrl, providerId), + {}, + { timeoutMs: 8000, maxRetries: 0 }, + ); + if (!res.ok) return []; + const data = (await res.json()) as { + data?: Array<{ id: string }>; + }; + return (data.data ?? []).map((m) => m.id).filter(Boolean); + } catch { + return []; + } + }; + + const outcome = await runWizard({ + providers: wizardProviders, + pickProvider: async (options) => { + const picked = await vscode.window.showQuickPick( + options.map((p) => ({ + label: p.id, + description: p.cloud ? "cloud API" : "self-hosted", + })), + { placeHolder: "Choose a provider to configure" }, + ); + return picked?.label; + }, + inputBaseUrl: async (providerId) => { + const value = await vscode.window.showInputBox({ + prompt: `Base URL for ${providerId}`, + value: DEFAULT_BASE_URL[providerId] ?? "", + ignoreFocusOut: true, + }); + return value?.trim() || undefined; + }, + probeEndpoint: async (providerId, baseUrl) => { + if (CLOUD_PROVIDERS.has(providerId)) { + // Deterministic decision: cloud without a stored key behaves as + // 401 so the wizard captures the key; cloud with a key is "ready". + const hasKey = await context.secrets.get(SECRET_KEY[providerId]); + return hasKey ? { status: 200 } : { status: 401 }; + } + try { + const res = await resilientFetch( + buildProbeUrl(baseUrl, providerId), + {}, + { timeoutMs: 6000, maxRetries: 1 }, + ); + return { status: res.status }; + } catch { + return { status: undefined, ok: false }; + } + }, + hasStoredApiKey: async (providerId) => + Boolean(await context.secrets.get(SECRET_KEY[providerId] ?? "")), + promptForApiKey: async (providerId) => { + const key = await vscode.window.showInputBox({ + prompt: `${providerId} requires an API key. Paste it now (stored in VS Code's secure secret store):`, + password: true, + ignoreFocusOut: true, + }); + return key?.trim() || undefined; + }, + storeKey: async (providerId, apiKey) => { + await context.secrets.store(SECRET_KEY[providerId], apiKey); + }, + fetchModels: listModels, + pickModel: async (models) => { + const picked = await vscode.window.showQuickPick(models, { + placeHolder: "Choose a model", + ignoreFocusOut: true, + }); + return picked ?? undefined; + }, + }); + + if (outcome.status === "cancelled") return; + + const activeFolder = resolveActiveWorkspaceFolder() ?? workspaceRoot; + const targetDirUri = vscode.Uri.file( + path.join(activeFolder ?? os.homedir(), ".champ"), + ); + const targetUri = vscode.Uri.file( + path.join(targetDirUri.fsPath, "config.yaml"), + ); + let previousText: string | null = null; + try { + previousText = new TextDecoder().decode( + await vscode.workspace.fs.readFile(targetUri), + ); + } catch { + previousText = null; // no file yet + } + const { yaml: updated } = upsertProviderInYaml( + previousText, + { + providerId: outcome.providerId, + baseUrl: outcome.baseUrl, + model: outcome.model, + }, + { setActive: true }, + ); + try { + await vscode.workspace.fs.createDirectory(targetDirUri); + } catch { + // already exists + } + await vscode.workspace.fs.writeFile( + targetUri, + new TextEncoder().encode(updated), + ); + await loadProvider(); + void vscode.window.showInformationMessage( + `Champ: ${outcome.providerId} configured and active.`, + ); + }), vscode.commands.registerCommand("champ.about", () => { const version = context.extension.packageJSON.version as string; const provider = inlineProviderRef.current; @@ -1907,7 +2079,7 @@ export async function activate( channel.appendLine(` user : ${userPath}`); if (result.ignoredSources.length > 0) { channel.appendLine( - `Ignored : ${result.ignoredSources.join(", ")} (champ.configSource)`, + `Ignored : ${result.ignoredSources.join(", ")} (workspace > user)`, ); } channel.appendLine(""); @@ -3146,18 +3318,17 @@ export async function activate( }), ); - // ---- Config loader (YAML + VS Code settings fallback) ------------- + // ---- Config loader (YAML-only since #118) ----------------------- /** * Resolve the effective ChampConfig from (in order of precedence): * 1. /.champ/config.yaml * 2. ~/.champ/config.yaml - * 3. VS Code champ.* settings (legacy backward-compat) - * 4. built-in defaults + * 3. built-in defaults * * Returns null when no source has a usable config — the loader path - * is then skipped and the caller falls back to createFromConfig(). - * Errors during YAML parsing are surfaced to the user but do not - * crash activation. + * is then skipped and loadProvider surfaces the "run Champ: Configure + * Provider" hint. Errors during YAML parsing are surfaced to the user + * but do not crash activation. */ // Last layered-resolution outcome, for Show Effective Config (#115). let lastLayeredInfo: { @@ -3187,18 +3358,14 @@ export async function activate( } }; - const source = vscode.workspace - .getConfiguration("champ") - .get< - "auto" | "workspace-yaml" | "user-yaml" | "settings" - >("configSource", "auto"); - + // YAML is the only source of truth since #118: favour the workspace + // .champ/config.yaml, then ~/.champ/config.yaml. let result: import("./config/config-loader").LayeredResult; try { result = resolveLayered({ workspaceText: await readRaw(workspacePath), userText: await readRaw(userPath), - source, + source: "auto", }); } catch (err) { void vscode.window.showErrorMessage( @@ -3211,13 +3378,13 @@ export async function activate( if (result.ignoredSources.length > 0) { console.log( - `Champ config: ignoring ${result.ignoredSources.join(", ")} because configSource=${source}`, + `Champ config: ignoring ${result.ignoredSources.join(", ")} (auto precedence)`, ); } if (result.conflict && !conflictNoticeShown) { conflictNoticeShown = true; void vscode.window.showInformationMessage( - `Champ: workspace .champ/config.yaml overrides ~/.champ/config.yaml. Set "champ.configSource" to pick one explicitly.`, + `Champ: workspace .champ/config.yaml overrides ~/.champ/config.yaml.`, ); } @@ -3234,8 +3401,9 @@ export async function activate( * On failure, leaves the stub in place and surfaces the error in the * chat panel + status bar. * - * Tries the YAML config path first, falling back to legacy - * VS Code settings if no YAML config is present. + * YAML config (.champ/config.yaml, then ~/.champ/config.yaml) is + * the only source since #118. If none is present, surfaces the + * "Run Champ: Configure Provider" hint. * * Concurrent calls are deduplicated — the second caller receives * the same promise as the first, preventing config/race corruption. @@ -3283,7 +3451,7 @@ export async function activate( ? yamlConfig?.providers?.[activeName]?.model : undefined; console.log( - `Champ config: source=${result.usedSource} provider=${activeName ?? "(settings)"} model=${activeModel ?? "(settings)"}`, + `Champ config: source=${result.usedSource} provider=${activeName ?? "(none)"} model=${activeModel ?? "(none)"}`, ); } // Load project rules from .champ/rules/*.md @@ -3315,10 +3483,11 @@ export async function activate( }); const rawProvider = yamlConfig ? await factory.createFromChampConfig(yamlConfig, context.secrets) - : await factory.createFromConfig( - vscode.workspace.getConfiguration("champ"), - context.secrets, - ); + : (() => { + throw new Error( + "No Champ configuration found. Run 'Champ: Configure Provider' or create .champ/config.yaml.", + ); + })(); // Wrap primary provider in a CircuitBreaker so repeated failures stop // hammering a known-bad endpoint. const wrappedPrimary = new CircuitBreaker(rawProvider); diff --git a/src/providers/factory.ts b/src/providers/factory.ts index 38b9266..d681a6c 100644 --- a/src/providers/factory.ts +++ b/src/providers/factory.ts @@ -1,10 +1,8 @@ /** - * ProviderFactory: builds an LLMProvider from VS Code settings. + * ProviderFactory: builds an LLMProvider from the YAML ChampConfig. * - * Encapsulates the logic of reading `champ.*` configuration values - * plus API keys from SecretStorage and instantiating the right - * provider subclass. The extension activation code calls this once - * at startup and whenever the user changes the active provider. + * Since #118 YAML is the only source of truth. API keys come from + * SecretStorage (cloud) or the YAML `apiKey` field (self-hosted). */ import type { LLMProvider, LLMProviderConfig } from "./types"; import { ClaudeProvider } from "./claude"; @@ -16,15 +14,6 @@ import { VLLMProvider } from "./vllm"; import { OpenAICompatibleProvider } from "./openai-compatible"; import type { ChampConfig } from "../config/config-loader"; -/** - * Narrow interface matching the subset of vscode.WorkspaceConfiguration - * we actually use. Accepted as a parameter so tests can pass a plain - * object without importing vscode. - */ -export interface ConfigReader { - get(section: string): T | undefined; -} - /** * Narrow interface matching vscode.SecretStorage. Tests provide a fake. * Uses PromiseLike so vscode.SecretStorage's Thenable return is @@ -34,66 +23,13 @@ export interface SecretReader { get(key: string): PromiseLike; } -/** - * Config defaults applied when the user hasn't set the value. - */ -const DEFAULTS: Record = { - "claude.model": "claude-sonnet-4-20250514", - "openai.model": "gpt-4o", - "gemini.model": "gemini-2.0-flash", - "ollama.baseUrl": "http://localhost:11434", - "ollama.model": "llama3.1", - "llamacpp.baseUrl": "http://localhost:8080/v1", - "llamacpp.model": "default", - "vllm.baseUrl": "http://localhost:8000/v1", -}; - export class ProviderFactory { - /** - * Build a provider instance from the current configuration snapshot. - * - * @param config - VS Code configuration (or a fake in tests) - * @param secrets - SecretStorage (or a fake in tests) - */ - async createFromConfig( - config: ConfigReader, - secrets: SecretReader, - ): Promise { - const providerId = this.read(config, "provider") ?? "claude"; - - switch (providerId) { - case "claude": - return this.createClaude(config, secrets); - case "openai": - return this.createOpenAI(config, secrets); - case "gemini": - return this.createGemini(config, secrets); - case "ollama": - return this.createOllama(config); - case "llamacpp": - return this.createLlamaCpp(config); - case "vllm": - return this.createVLLM(config, secrets); - case "openai-compatible": - return this.createOpenAICompatible(config, secrets); - default: - throw new Error(`Unknown provider: "${providerId}"`); - } - } - - private read(config: ConfigReader, section: string): T | undefined { - const value = config.get(section); - if (value !== undefined && value !== null && value !== "") return value; - return DEFAULTS[section] as T | undefined; - } - /** * Build a provider instance from a parsed ChampConfig (YAML-based). * - * Unlike createFromConfig() which reads flat champ.* keys from VS - * Code's settings.json, this path takes a structured ChampConfig - * (typically loaded from .champ/config.yaml). API keys still come - * from SecretStorage — never from YAML. + * This path takes a structured ChampConfig (typically loaded from + * .champ/config.yaml). API keys come from SecretStorage or env vars + * — never from YAML for cloud providers. */ async createFromChampConfig( config: ChampConfig, @@ -213,87 +149,4 @@ export class ProviderFactory { ...extras, }; } - - private async createClaude( - config: ConfigReader, - secrets: SecretReader, - ): Promise { - const apiKey = await secrets.get("champ.claude.apiKey"); - return new ClaudeProvider({ - ...this.baseConfig("claude"), - model: - this.read(config, "claude.model") ?? "claude-sonnet-4-20250514", - apiKey, - }); - } - - private async createOpenAI( - config: ConfigReader, - secrets: SecretReader, - ): Promise { - const apiKey = await secrets.get("champ.openai.apiKey"); - return new OpenAIProvider({ - ...this.baseConfig("openai"), - model: this.read(config, "openai.model") ?? "gpt-4o", - apiKey, - }); - } - - private async createGemini( - config: ConfigReader, - secrets: SecretReader, - ): Promise { - const apiKey = await secrets.get("champ.gemini.apiKey"); - return new GeminiProvider({ - ...this.baseConfig("gemini"), - model: this.read(config, "gemini.model") ?? "gemini-2.0-flash", - apiKey, - }); - } - - private createOllama(config: ConfigReader): LLMProvider { - return new OllamaProvider({ - ...this.baseConfig("ollama"), - model: this.read(config, "ollama.model") ?? "llama3.1", - baseUrl: - this.read(config, "ollama.baseUrl") ?? "http://localhost:11434", - }); - } - - private createLlamaCpp(config: ConfigReader): LLMProvider { - return new LlamaCppProvider({ - ...this.baseConfig("llamacpp"), - model: this.read(config, "llamacpp.model") ?? "default", - baseUrl: - this.read(config, "llamacpp.baseUrl") ?? - "http://localhost:8080/v1", - }); - } - - private async createVLLM( - config: ConfigReader, - secrets: SecretReader, - ): Promise { - const apiKey = await secrets.get("champ.vllm.apiKey"); - return new VLLMProvider({ - ...this.baseConfig("vllm"), - model: this.read(config, "vllm.model") ?? "", - baseUrl: - this.read(config, "vllm.baseUrl") ?? "http://localhost:8000/v1", - apiKey, - }); - } - - private async createOpenAICompatible( - config: ConfigReader, - secrets: SecretReader, - ): Promise { - const apiKey = await secrets.get("champ.openaiCompatible.apiKey"); - return new OpenAICompatibleProvider({ - ...this.baseConfig("openai-compatible"), - model: this.read(config, "openaiCompatible.model") ?? "default", - baseUrl: this.read(config, "openaiCompatible.baseUrl") ?? "", - apiKey, - }); - } } diff --git a/test/unit/config/layered-source.test.ts b/test/unit/config/layered-source.test.ts index b368bb1..9b476a6 100644 --- a/test/unit/config/layered-source.test.ts +++ b/test/unit/config/layered-source.test.ts @@ -1,11 +1,11 @@ /** - * TDD: layered config resolution with explicit single source (#115). + * TDD: layered config resolution (#115 / #118). * - * Contract: - * - champ.configSource picks THE one active source; others are ignored - * and reported via ignoredSources. - * - auto preserves historical precedence exactly: - * workspace yaml > user yaml > (caller's settings fallback) + * Contract (YAML-only since #118): + * - `.champ/config.yaml` (workspace) > `~/.champ/config.yaml` (user). + * The old `champ.configSource` setting and the legacy settings layer are + * gone — YAML is the single source of truth. + * - ignoredSources reports which files were deliberately not consulted. * - origins maps every top-level key of the final config to the layer * that supplied it ("workspace-yaml" | "user-yaml" | "default"). */ @@ -52,13 +52,13 @@ describe("resolveLayered (#115)", () => { expect(r.conflict).toBe(false); }); - it("auto: null config when no yaml at all (settings fallback)", () => { + it("auto: null config when no yaml at all (no settings fallback, #118)", () => { const r = resolveLayered({ workspaceText: null, userText: null, source: "auto", }); - expect(r.usedSource).toBe("settings"); + expect(r.usedSource).toBe("default"); expect(r.config).toBeNull(); }); @@ -85,17 +85,15 @@ describe("resolveLayered (#115)", () => { expect(r.ignoredSources).toContain("workspace-yaml"); }); - it("explicit settings skips all yaml files", () => { + it("unknown source: the 'settings' option no longer exists (#118)", () => { const r = resolveLayered({ workspaceText: WS_YAML, userText: USER_YAML, - source: "settings", + source: "auto", }); - expect(r.usedSource).toBe("settings"); - expect(r.config).toBeNull(); - expect(r.ignoredSources).toEqual( - expect.arrayContaining(["workspace-yaml", "user-yaml"]), - ); + expect(r.usedSource).toBe("workspace-yaml"); + // settings is not a config source anymore — YAML is the only store. + expect(["workspace-yaml", "user-yaml", "default"]).toContain(r.usedSource); }); it("origins attribute each top-level key to its layer", () => { diff --git a/test/unit/config/setup-wizard.test.ts b/test/unit/config/setup-wizard.test.ts new file mode 100644 index 0000000..cf069e5 --- /dev/null +++ b/test/unit/config/setup-wizard.test.ts @@ -0,0 +1,123 @@ +/** + * TDD: guided setup wizard flow controller (ticket #118). + * Decision rules: + * - endpoint probe 401 → prompt for and store an API key + * - cloud provider with stored key → keep the current model, skip picking + * - otherwise pick from the auto-discovered model list + */ +import { describe, it, expect, vi } from "vitest"; +import { + runWizard, + type WizardDeps, + type WizardOutcome, +} from "@/config/setup-wizard"; + +const baseDeps = (overrides: Partial = {}): WizardDeps => ({ + providers: [ + { id: "claude", cloud: true }, + { id: "ollama", cloud: false }, + ], + pickProvider: vi.fn(async () => "ollama"), + inputBaseUrl: vi.fn(async () => "http://localhost:11434"), + probeEndpoint: vi.fn(async () => ({ status: 200 })), + hasStoredApiKey: vi.fn(async () => false), + promptForApiKey: vi.fn(async () => "sk-test"), + storeKey: vi.fn(async () => {}), + fetchModels: vi.fn(async () => ["llama3.1", "qwen3:8b"]), + pickModel: vi.fn(async () => "qwen3:8b"), + ...overrides, +}); + +const run = async (deps: WizardDeps): Promise => runWizard(deps); + +describe("runWizard (#118)", () => { + it("returns cancelled when no provider is picked", async () => { + const outcome = await run( + baseDeps({ pickProvider: vi.fn(async () => undefined) }), + ); + expect(outcome.status).toBe("cancelled"); + }); + + it("returns cancelled when baseUrl is not supplied", async () => { + const outcome = await run( + baseDeps({ inputBaseUrl: vi.fn(async () => undefined) }), + ); + expect(outcome.status).toBe("cancelled"); + }); + + it("prompts for and stores a key when the probe returns 401", async () => { + const storeKey = vi.fn(async () => {}); + const outcome = await run( + baseDeps({ + probeEndpoint: vi.fn(async () => ({ status: 401 })), + storeKey, + }), + ); + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.apiKey).toBe("sk-test"); + expect(storeKey).toHaveBeenCalledWith("ollama", "sk-test"); + }); + + it("does NOT prompt for a key when the probe succeeds", async () => { + const promptForApiKey = vi.fn(async () => "sk-test"); + const outcome = await run(baseDeps({ promptForApiKey })); + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(promptForApiKey).not.toHaveBeenCalled(); + expect(outcome.apiKey).toBeUndefined(); + }); + + it("auto-keeps the configured model for a cloud provider with a stored key", async () => { + const fetchModels = vi.fn(async () => ["claude-sonnet-4-20250514"]); + const pickModel = vi.fn(async () => "gpt-4o"); + const outcome = await run( + baseDeps({ + pickProvider: vi.fn(async () => "claude"), + hasStoredApiKey: vi.fn(async () => true), + fetchModels, + pickModel, + }), + ); + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(fetchModels).not.toHaveBeenCalled(); + expect(pickModel).not.toHaveBeenCalled(); + expect(outcome.model).toBeUndefined(); + }); + + it("picks from discovered models for a self-hosted provider", async () => { + const outcome = await run(baseDeps()); + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.model).toBe("qwen3:8b"); + expect(outcome.baseUrl).toBe("http://localhost:11434"); + }); + + it("auto-selects the only discovered model", async () => { + const pickModel = vi.fn(async () => "qwen3:8b"); + const outcome = await run( + baseDeps({ + fetchModels: vi.fn(async () => ["qwen3:8b"]), + pickModel, + }), + ); + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(pickModel).not.toHaveBeenCalled(); + expect(outcome.model).toBe("qwen3:8b"); + }); + + it("requests a key for a cloud provider without one whenever the probe 401s", async () => { + const outcome = await run( + baseDeps({ + pickProvider: vi.fn(async () => "claude"), + hasStoredApiKey: vi.fn(async () => false), + probeEndpoint: vi.fn(async () => ({ status: 401 })), + }), + ); + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.apiKey).toBe("sk-test"); + }); +}); diff --git a/test/unit/config/yaml-writer.test.ts b/test/unit/config/yaml-writer.test.ts new file mode 100644 index 0000000..00bdb93 --- /dev/null +++ b/test/unit/config/yaml-writer.test.ts @@ -0,0 +1,164 @@ +/** + * TDD: YAML merge-writer for guided setup (ticket #118). + * The wizard updates one provider block in .champ/config.yaml while + * preserving every other hand-edited section. + */ +import { describe, it, expect } from "vitest"; +import * as yamlJs from "js-yaml"; +import { + upsertProviderInYaml, + providerExistsInYaml, + type ProviderYamlPatch, +} from "@/config/yaml-writer"; + +const buildBlock = (providerId: string, overrides = {}): ProviderYamlPatch => ({ + providerId, + baseUrl: "http://192.168.1.40:8000/v1", + model: "code-model-7b", + ...overrides, +}); + +describe("upsertProviderInYaml (#118)", () => { + it("creates a fresh config when the file is empty", () => { + const { yaml, created } = upsertProviderInYaml("", buildBlock("ollama")); + const parsed = yamlJs.load(yaml) as Record; + expect(created).toBe(true); + expect(parsed.providers).toEqual({ + ollama: { + baseUrl: "http://192.168.1.40:8000/v1", + model: "code-model-7b", + }, + }); + }); + + it("sets the active provider when asked", () => { + const { yaml } = upsertProviderInYaml("", buildBlock("ollama"), { + setActive: true, + }); + const parsed = yamlJs.load(yaml) as Record; + expect(parsed.provider).toBe("ollama"); + }); + + it("preserves unrelated sections when adding a provider", () => { + const existing = ` +provider: claude +providers: + claude: + model: claude-sonnet-4-20250514 +autocomplete: + enabled: true + debounceMs: 300 +agent: + yoloMode: true +`; + const { yaml } = upsertProviderInYaml(existing, buildBlock("llamacpp")); + const parsed = yamlJs.load(yaml) as Record; + expect(parsed.autocomplete).toEqual({ enabled: true, debounceMs: 300 }); + expect(parsed.agent).toEqual({ yoloMode: true }); + expect((parsed.providers as Record).claude).toEqual({ + model: "claude-sonnet-4-20250514", + }); + expect( + (parsed.providers as Record).llamacpp, + ).toMatchObject({ model: "code-model-7b" }); + }); + + it("updates an existing provider block without touching others", () => { + const existing = ` +provider: ollama +providers: + ollama: + baseUrl: http://localhost:11434 + model: llama3.1 + claude: + model: claude-sonnet-4-20250514 +`; + const { yaml } = upsertProviderInYaml( + existing, + { + providerId: "ollama", + baseUrl: "http://192.168.1.99:11434", + model: "qwen3:8b", + }, + { setActive: true }, + ); + const parsed = yamlJs.load(yaml) as { + provider: string; + providers: { + ollama: { baseUrl: string; model: string }; + claude: unknown; + }; + }; + expect(parsed.provider).toBe("ollama"); + expect(parsed.providers.ollama).toEqual({ + baseUrl: "http://192.168.1.99:11434", + model: "qwen3:8b", + }); + expect(parsed.providers.claude).toEqual({ + model: "claude-sonnet-4-20250514", + }); + }); + + it("writes the options and contextWindow block", () => { + const { yaml } = upsertProviderInYaml( + "", + buildBlock("ollama", { + contextWindow: 16384, + options: { temperature: 0.25, topP: 0.95, seed: 42 }, + }), + ); + const parsed = yamlJs.load(yaml) as { + providers: { + ollama: { + contextWindow: number; + options: { temperature: number; seed: number }; + }; + }; + }; + expect(parsed.providers.ollama.contextWindow).toBe(16384); + expect(parsed.providers.ollama.options).toEqual({ + temperature: 0.25, + topP: 0.95, + seed: 42, + }); + }); + + it("merges into an existing block (old keys kept, new ones added)", () => { + const existing = ` +providers: + ollama: + baseUrl: http://localhost:11434 + model: llama3.1 +`; + const { yaml } = upsertProviderInYaml(existing, { + providerId: "ollama", + model: "qwen3:8b", + options: { temperature: 0.2 }, + }); + const parsed = yamlJs.load(yaml) as { + providers: { ollama: Record }; + }; + // baseUrl untouched, model replaced, options added + expect(parsed.providers.ollama.baseUrl).toBe("http://localhost:11434"); + expect(parsed.providers.ollama.model).toBe("qwen3:8b"); + expect(parsed.providers.ollama.options).toEqual({ temperature: 0.2 }); + }); +}); + +describe("providerExistsInYaml (#118)", () => { + it("detects an existing provider", () => { + const existing = ` +provider: ollama +providers: + ollama: + model: llama3.1 +`; + expect(providerExistsInYaml(existing, "ollama")).toBe(true); + expect(providerExistsInYaml(existing, "llamacpp")).toBe(false); + }); + + it("returns false for empty input", () => { + expect(providerExistsInYaml("", "ollama")).toBe(false); + expect(providerExistsInYaml(undefined, "ollama")).toBe(false); + }); +}); diff --git a/test/unit/providers/factory.test.ts b/test/unit/providers/factory.test.ts index 3abee81..1dd631b 100644 --- a/test/unit/providers/factory.test.ts +++ b/test/unit/providers/factory.test.ts @@ -1,23 +1,14 @@ /** - * TDD: Tests for ProviderFactory. - * Reads VS Code config + secret storage, returns an LLMProvider. + * TDD: Tests for ProviderFactory (YAML-only since #118). + * Reads a ChampConfig + secret storage, returns an LLMProvider. */ import { describe, it, expect, beforeEach, vi } from "vitest"; import { ProviderFactory } from "@/providers/factory"; /** - * Minimal fake for vscode.WorkspaceConfiguration and SecretStorage so - * tests don't depend on the extension host. + * Minimal fake for vscode.SecretStorage so tests don't depend on the + * extension host. */ -function createFakeConfig(values: Record) { - return { - get: vi.fn((key: string) => values[key]), - has: vi.fn((key: string) => key in values), - update: vi.fn(), - inspect: vi.fn(), - }; -} - function createFakeSecrets(values: Record) { return { get: vi.fn(async (key: string) => values[key]), @@ -33,105 +24,6 @@ describe("ProviderFactory", () => { factory = new ProviderFactory(); }); - it("creates a ClaudeProvider from config", async () => { - const config = createFakeConfig({ - provider: "claude", - "claude.model": "claude-sonnet-4-20250514", - }) as never; - const secrets = createFakeSecrets({ - "champ.claude.apiKey": "test-key", - }) as never; - - const provider = await factory.createFromConfig(config, secrets); - expect(provider.name).toBe("claude"); - expect(provider.config.model).toBe("claude-sonnet-4-20250514"); - }); - - it("creates an OpenAIProvider from config", async () => { - const config = createFakeConfig({ - provider: "openai", - "openai.model": "gpt-4o", - }) as never; - const secrets = createFakeSecrets({ - "champ.openai.apiKey": "sk-test", - }) as never; - - const provider = await factory.createFromConfig(config, secrets); - expect(provider.name).toBe("openai"); - expect(provider.config.model).toBe("gpt-4o"); - }); - - it("creates a GeminiProvider from config", async () => { - const config = createFakeConfig({ - provider: "gemini", - "gemini.model": "gemini-2.0-flash", - }) as never; - const secrets = createFakeSecrets({ - "champ.gemini.apiKey": "key", - }) as never; - - const provider = await factory.createFromConfig(config, secrets); - expect(provider.name).toBe("gemini"); - }); - - it("creates an OllamaProvider without API key", async () => { - const config = createFakeConfig({ - provider: "ollama", - "ollama.baseUrl": "http://localhost:11434", - "ollama.model": "llama3.1", - }) as never; - const secrets = createFakeSecrets({}) as never; - - const provider = await factory.createFromConfig(config, secrets); - expect(provider.name).toBe("ollama"); - expect(provider.config.baseUrl).toBe("http://localhost:11434"); - }); - - it("creates a LlamaCppProvider", async () => { - const config = createFakeConfig({ - provider: "llamacpp", - "llamacpp.baseUrl": "http://localhost:8080/v1", - "llamacpp.model": "default", - }) as never; - const secrets = createFakeSecrets({}) as never; - - const provider = await factory.createFromConfig(config, secrets); - expect(provider.name).toBe("llamacpp"); - }); - - it("creates a VLLMProvider", async () => { - const config = createFakeConfig({ - provider: "vllm", - "vllm.baseUrl": "http://localhost:8000/v1", - "vllm.model": "meta-llama/Llama-3.1-8B", - }) as never; - const secrets = createFakeSecrets({}) as never; - - const provider = await factory.createFromConfig(config, secrets); - expect(provider.name).toBe("vllm"); - }); - - it("creates an openai-compatible provider", async () => { - const config = createFakeConfig({ - provider: "openai-compatible", - "openaiCompatible.baseUrl": "http://custom-server:9000/v1", - "openaiCompatible.model": "my-model", - }) as never; - const secrets = createFakeSecrets({ - "champ.openaiCompatible.apiKey": "key", - }) as never; - - const provider = await factory.createFromConfig(config, secrets); - expect(provider.name).toBe("openai-compatible"); - }); - - it("throws for unknown provider", async () => { - const config = createFakeConfig({ provider: "nonsense" }) as never; - const secrets = createFakeSecrets({}) as never; - - await expect(factory.createFromConfig(config, secrets)).rejects.toThrow(); - }); - describe("createFromChampConfig (YAML path)", () => { it("creates a llamacpp provider from a parsed ChampConfig", async () => { const provider = await factory.createFromChampConfig( From a2ce4729d65c9c2f2e60df0466797a236b904852 Mon Sep 17 00:00:00 2001 From: hellodk Date: Fri, 28 Aug 2026 15:32:36 +0530 Subject: [PATCH 5/5] feat: assistant-prefill for prompt-based tools, structured output + KV-cache opt-ins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prompt-based local models often narrate ('Let me check that file…') instead of emitting a tool call, wasting a round-trip and polluting history with fake results. On tool-continuation turns we now append a trailing assistant seed (the opener plus ) so the backend begins its next output inside the XML format. If the model answers with prose instead of completing a call, the seed is stripped before it reaches history or the UI. Also adds two per-provider YAML opt-ins: - structuredOutput: pins response_format/format json on chat bodies, never on tool-call turns (the XML tool prompt needs free text) - cachePrompt: sends options.cache_prompt so the backend keeps the processed prompt across turns instead of recomputing the full prefix Closes #121 --- src/agent/agent-controller.ts | 42 +- src/config/config-loader.ts | 21 + src/providers/factory.ts | 43 +- src/providers/ollama.ts | 11 +- src/providers/openai-compatible.ts | 7 + src/providers/prompt-based-tools.ts | 21 +- src/providers/types.ts | 13 + .../prefill-structured-output.test.ts | 399 ++++++++++++++++++ 8 files changed, 526 insertions(+), 31 deletions(-) create mode 100644 test/unit/providers/prefill-structured-output.test.ts diff --git a/src/agent/agent-controller.ts b/src/agent/agent-controller.ts index d04c628..3607829 100644 --- a/src/agent/agent-controller.ts +++ b/src/agent/agent-controller.ts @@ -32,6 +32,8 @@ import { extractTextContent, extractPreToolText, hasFabricatedNarration, + TOOL_CALL_START_PREFIX, + buildPrefillAssistantMessage, type MalformedToolCall, } from "../providers/prompt-based-tools"; import { SecretScanner } from "../safety/secret-scanner"; @@ -554,6 +556,13 @@ export class AgentController { if (options.abortSignal?.aborted) break; iterationRan = true; + // Assistant-prefill (#121): on prompt-based continuation turns (there + // is at least one tool registered and we have already run at least one + // iteration), a trailing assistant seed makes the model begin its next + // output inside the XML format instead of narrating. + const prefillNeeded = + usePromptBased && iteration > 0 && allTools.length > 0; + // Emit iteration start event so the UI can display live progress. this.emitIterationStart( iteration, @@ -670,13 +679,29 @@ export class AgentController { break; } - const stream = activeProvider.chat(messagesToSend, { + // Append the assistant-prefill seed on continuation turns (#121). + // Kept as a separate list so `messagesToSend` (the cache key) stays + // stable. Iteration 0 is never prefilled, so cache behaviour is + // unchanged; continuation turns are never cached anyway. + const requestMessages = prefillNeeded + ? [...messagesToSend, buildPrefillAssistantMessage()] + : messagesToSend; + + const stream = activeProvider.chat(requestMessages, { // Native tool defs only when the provider says it supports them. tools: usePromptBased ? undefined : allTools, abortSignal: options.abortSignal, taskHint: "coding", }); + // The backend continues from the seed we appended, so the buffered + // assistant text must start there too — otherwise the streamed output + // is just the tail of the tag and the assembled tool call would be + // missing its opener. + if (prefillNeeded) { + assistantText = TOOL_CALL_START_PREFIX; + } + let errorOccurred = false; for await (const delta of stream) { if (delta.type === "text" && delta.text) { @@ -752,6 +777,21 @@ export class AgentController { malformedCalls.push(info), ); + // Assistant-prefill answered with prose (#121): if the seeded + // continuation turn produced a plain text reply instead of a + // complete tool call, drop the dangling seed so it never leaks + // into history or the UI. The parse above already ran on the + // seeded text, so this only fires when no call was completed — + // the seed is pure noise at that point. + if ( + prefillNeeded && + parsed.length === 0 && + malformedCalls.length === 0 && + assistantText.startsWith(TOOL_CALL_START_PREFIX) + ) { + assistantText = assistantText.slice(TOOL_CALL_START_PREFIX.length); + } + // ── Hallucination guard (issue #101) ──────────────────────────── // If no tool calls were parsed but the text contains fabricated // narration (e.g. "---Result of reading documentation:"), the diff --git a/src/config/config-loader.ts b/src/config/config-loader.ts index 3837ee8..8a746cc 100644 --- a/src/config/config-loader.ts +++ b/src/config/config-loader.ts @@ -56,6 +56,10 @@ export interface ProviderConfig { apiKey?: string; /** Opt-in to native OpenAI tool calling (openai-compatible spec servers). */ supportsTools?: boolean; + /** Opt-in to JSON-constrained generation on chat requests (#121). */ + structuredOutput?: boolean; + /** Ask the backend to keep its prompt/KV cache warm (#121). */ + cachePrompt?: boolean; /** * Cap the effective context window for this provider (tokens). Never raise * above what config pins, even if the runtime advertises more. Ticket #119. @@ -433,6 +437,23 @@ export class ConfigLoader { pc.supportsTools = c.supportsTools; } } + if ("structuredOutput" in c) { + // Opt-in to JSON-constrained generation (#121). Rejected on + // tool-call turns; the XML tool prompt needs free text. + if (typeof c.structuredOutput !== "boolean") { + pushError(`providers.${name}.structuredOutput must be a boolean`); + } else { + pc.structuredOutput = c.structuredOutput; + } + } + if ("cachePrompt" in c) { + // Ask the backend to keep its prompt/KV cache warm (#121). + if (typeof c.cachePrompt !== "boolean") { + pushError(`providers.${name}.cachePrompt must be a boolean`); + } else { + pc.cachePrompt = c.cachePrompt; + } + } if ("contextWindow" in c) { const cw = c as Record; if ( diff --git a/src/providers/factory.ts b/src/providers/factory.ts index d681a6c..767230f 100644 --- a/src/providers/factory.ts +++ b/src/providers/factory.ts @@ -38,6 +38,14 @@ export class ProviderFactory { const providerName = config.provider ?? "claude"; const providerEntry = config.providers?.[providerName] ?? {}; + // Shared config members threaded into every provider's baseConfig (#121). + const baseExtras: Partial> = { + contextWindow: providerEntry.contextWindow, + options: providerEntry.options, + structuredOutput: providerEntry.structuredOutput, + cachePrompt: providerEntry.cachePrompt, + }; + // Helper: try SecretStorage first, then environment variable. const getKey = async ( secretKey: string, @@ -51,37 +59,25 @@ export class ProviderFactory { switch (providerName) { case "claude": return new ClaudeProvider({ - ...this.baseConfig("claude", { - contextWindow: providerEntry.contextWindow, - options: providerEntry.options, - }), + ...this.baseConfig("claude", baseExtras), model: providerEntry.model ?? "claude-sonnet-4-20250514", apiKey: await getKey("champ.claude.apiKey", "ANTHROPIC_API_KEY"), }); case "openai": return new OpenAIProvider({ - ...this.baseConfig("openai", { - contextWindow: providerEntry.contextWindow, - options: providerEntry.options, - }), + ...this.baseConfig("openai", baseExtras), model: providerEntry.model ?? "gpt-4o", apiKey: await getKey("champ.openai.apiKey", "OPENAI_API_KEY"), }); case "gemini": return new GeminiProvider({ - ...this.baseConfig("gemini", { - contextWindow: providerEntry.contextWindow, - options: providerEntry.options, - }), + ...this.baseConfig("gemini", baseExtras), model: providerEntry.model ?? "gemini-2.0-flash", apiKey: await getKey("champ.gemini.apiKey", "GEMINI_API_KEY"), }); case "ollama": return new OllamaProvider({ - ...this.baseConfig("ollama", { - contextWindow: providerEntry.contextWindow, - options: providerEntry.options, - }), + ...this.baseConfig("ollama", baseExtras), model: providerEntry.model ?? "llama3.1", baseUrl: providerEntry.baseUrl ?? "http://localhost:11434", // apiKey from YAML (operator-issued) takes precedence over SecretStorage @@ -91,10 +87,7 @@ export class ProviderFactory { }); case "llamacpp": return new LlamaCppProvider({ - ...this.baseConfig("llamacpp", { - contextWindow: providerEntry.contextWindow, - options: providerEntry.options, - }), + ...this.baseConfig("llamacpp", baseExtras), model: providerEntry.model ?? "default", baseUrl: providerEntry.baseUrl ?? "http://localhost:8080/v1", apiKey: @@ -104,10 +97,7 @@ export class ProviderFactory { }); case "vllm": return new VLLMProvider({ - ...this.baseConfig("vllm", { - contextWindow: providerEntry.contextWindow, - options: providerEntry.options, - }), + ...this.baseConfig("vllm", baseExtras), model: providerEntry.model ?? "", baseUrl: providerEntry.baseUrl ?? "http://localhost:8000/v1", apiKey: @@ -117,10 +107,7 @@ export class ProviderFactory { }); case "openai-compatible": return new OpenAICompatibleProvider({ - ...this.baseConfig("openai-compatible", { - contextWindow: providerEntry.contextWindow, - options: providerEntry.options, - }), + ...this.baseConfig("openai-compatible", baseExtras), model: providerEntry.model ?? "default", baseUrl: providerEntry.baseUrl ?? "", apiKey: diff --git a/src/providers/ollama.ts b/src/providers/ollama.ts index 791fffb..ca93c43 100644 --- a/src/providers/ollama.ts +++ b/src/providers/ollama.ts @@ -262,6 +262,9 @@ export class OllamaProvider implements LLMProvider { ...(dec.seed !== undefined && { seed: dec.seed }), ...(dec.stop !== undefined && { stop: dec.stop }), num_predict: options?.maxTokens ?? this.config.maxTokens, + // KV/prompt-cache persistence (#121): keep the processed prompt + // across turns instead of recomputing the full prefix each request. + ...(this.config.cachePrompt && { cache_prompt: true }), }, tools: options?.tools?.map((t) => ({ type: "function", @@ -271,7 +274,13 @@ export class OllamaProvider implements LLMProvider { parameters: t.parameters, }, })), - ...(options?.jsonFormat && { format: "json" }), + // Structured output (#121): explicit request-level jsonFormat or the + // provider-config structuredOutput opt-in. Never constrains tool-call + // turns — the XML tool prompt needs free text. + ...((options?.jsonFormat || + (this.config.structuredOutput && options?.taskHint !== "toolcall")) && { + format: "json", + }), }; try { diff --git a/src/providers/openai-compatible.ts b/src/providers/openai-compatible.ts index 8178fd4..d5d042d 100644 --- a/src/providers/openai-compatible.ts +++ b/src/providers/openai-compatible.ts @@ -171,6 +171,13 @@ export class OpenAICompatibleProvider implements LLMProvider { parameters: t.parameters, }, })), + // Structured output (#121): provider-config structuredOutput opt-in + // pins JSON-object mode. Never constrains tool-call turns — the XML + // tool prompt needs free text. + ...(this.config.structuredOutput && + options?.taskHint !== "toolcall" && { + response_format: { type: "json_object" }, + }), }; // One attempt = fetch + stream parse. Some servers (omlx behind a diff --git a/src/providers/prompt-based-tools.ts b/src/providers/prompt-based-tools.ts index 9ed7f34..1ab9502 100644 --- a/src/providers/prompt-based-tools.ts +++ b/src/providers/prompt-based-tools.ts @@ -5,7 +5,7 @@ * (many small local models), we inject tool definitions into the system * prompt as XML and parse the model's text output for blocks. */ -import type { ToolDefinition, ToolCall } from "./types"; +import type { ToolDefinition, ToolCall, LLMMessage } from "./types"; /** * Injects tool definitions into a system prompt using an XML format that @@ -133,6 +133,25 @@ export interface MalformedToolCall { reason: string; } +/** + * Trailing seed appended to prompt-based tool-continuation turns (#121). + * + * After the model finishes a tool call and we come back around, the model is + * asked to continue from an assistant message seeded with the XML opener for + * its next ``. Small local models that would normally narrate + * ("Let me check that file…") instead begin inside the XML format, producing + * deterministic, parseable output without a narration round-trip. + * + * The backend continues generating after this seed, so an assistant message + * of exactly this content (no closing tags) is appended to the request. + */ +export const TOOL_CALL_START_PREFIX = "\n"; + +/** Builds the trailing assistant seed message appended to continuation turns. */ +export function buildPrefillAssistantMessage(): LLMMessage { + return { role: "assistant", content: TOOL_CALL_START_PREFIX }; +} + /** * Parse `` XML and Qwen-style tool tokens out of model text. * diff --git a/src/providers/types.ts b/src/providers/types.ts index 41667ca..142e995 100644 --- a/src/providers/types.ts +++ b/src/providers/types.ts @@ -167,6 +167,19 @@ export interface LLMProviderConfig { * llama.cpp server, LM Studio with a tool-capable model). */ supportsTools?: boolean; + /** + * Opt-in to JSON-constrained generation on chat requests (#121). When set, + * the provider pins its backend's structured-output field + * (`response_format: {type:"json_object"}` / `format: "json"`). Never + * applied on tool-call turns — the XML tool prompt needs free text. + */ + structuredOutput?: boolean; + /** + * Ask the backend to keep the prompt/KV cache warm between turns (#121). + * ollama: `options.cache_prompt: true`. Improves latency for long system + * prompts with little first-token cost. + */ + cachePrompt?: boolean; } /** diff --git a/test/unit/providers/prefill-structured-output.test.ts b/test/unit/providers/prefill-structured-output.test.ts new file mode 100644 index 0000000..fbd064e --- /dev/null +++ b/test/unit/providers/prefill-structured-output.test.ts @@ -0,0 +1,399 @@ +/** + * TDD: ticket #121 — assistant-prefill, prefix stability, structured output, + * and KV-prompt-cache toggle. + * + * F1 Assistant-prefill: on prompt-based tool-continuation turns the request + * carries a trailing assistant seed so local models begin inside the + * `` format instead of narrating. If the model answers without + * completing a call, the seed is stripped so it never pollutes history. + * F2 Prefix stability: identical system + tools always assemble byte-identical + * prompts so backend KV/prompt caches hit across turns. + * F3 Structured output: `structuredOutput: true` on a provider pins + * `response_format`/`format: json` on chat bodies (never on tool-call turns). + * F4 KV-cache toggle: `cachePrompt: true` sends `options.cache_prompt`. + */ +import { describe, it, expect, vi } from "vitest"; +import { + TOOL_CALL_START_PREFIX, + buildPrefillAssistantMessage, + injectToolsIntoPrompt, +} from "@/providers/prompt-based-tools"; +import { OllamaProvider } from "@/providers/ollama"; +import { OpenAICompatibleProvider } from "@/providers/openai-compatible"; +import { ConfigLoader } from "@/config/config-loader"; +import { AgentController } from "@/agent/agent-controller"; +import type { LLMMessage, LLMProvider, StreamDelta } from "@/providers/types"; +import type { ToolRegistry } from "@/tools/registry"; + +// ── shared fixtures ────────────────────────────────────────────────────────── + +function mockStream(chunks: object[]): ReadableStream { + const encoder = new TextEncoder(); + const parts = chunks + .map((c) => `data: ${JSON.stringify(c)}\n\n`) + .concat(["data: [DONE]\n\n"]); + return new ReadableStream({ + start(controller) { + for (const part of parts) controller.enqueue(encoder.encode(part)); + controller.close(); + }, + }); +} + +const OLLAMA_BASE = { + provider: "ollama", + model: "m", + baseUrl: "http://localhost:11434", + maxTokens: 256, + temperature: 0.7, +}; + +const OAI_BASE = { + provider: "openai-compatible", + model: "m", + baseUrl: "http://localhost:8000/v1", + maxTokens: 256, + temperature: 0.7, +}; + +/** Capture the JSON body of the first (only) non-probe chat request. */ +async function captureChatBody( + provider: LLMProvider, + options: Parameters[1] = {}, +): Promise> { + let captured: Record = {}; + const isOllama = provider.name === "ollama"; + const mockFetch = vi.fn( + async (url: string | URL | Request, init?: RequestInit) => { + const u = String(url); + if (!isOllama && (u.endsWith("/props") || u.endsWith("/models"))) { + // Context-detect probes — route them to a not-found response. + return { ok: false } as unknown as Response; + } + captured = JSON.parse((init as RequestInit)?.body as string); + if (isOllama) { + return { + ok: true, + body: mockStream([{ message: { content: "hi" }, done: true }]), + } as unknown as Response; + } + return { + ok: true, + body: mockStream([{ choices: [{ delta: { content: "hi" } }] }]), + } as unknown as Response; + }, + ); + vi.stubGlobal("fetch", mockFetch); + for await (const _ of provider.chat( + [{ role: "user", content: "hi" }], + options, + )) { + // drain + } + vi.unstubAllGlobals(); + return captured; +} + +// ── F1: prefill unit ───────────────────────────────────────────────────────── + +describe("#121 prefill primitives", () => { + it("TOOL_CALL_START_PREFIX is the exact XML tool-call opener", () => { + expect(TOOL_CALL_START_PREFIX).toBe("\n"); + }); + + it("buildPrefillAssistantMessage returns a trailing assistant message", () => { + const m = buildPrefillAssistantMessage(); + expect(m.role).toBe("assistant"); + expect(m.content).toBe("\n"); + }); +}); + +// ── F2: prefix stability ───────────────────────────────────────────────────── + +describe("#121 prefix stability (KV/prompt-cache reuse)", () => { + const tools = [ + { + name: "read_file", + description: "Read a file", + parameters: { + type: "object", + properties: { path: { type: "string" } }, + required: ["path"], + }, + }, + { + name: "grep_search", + description: "Search for text", + parameters: { + type: "object", + properties: { + query: { type: "string" }, + include_pattern: { type: "string" }, + }, + required: ["query"], + }, + }, + ] as unknown as Parameters[1]; + + it("two turns with identical system+tools produce byte-identical prompts", () => { + const a = injectToolsIntoPrompt("Base system.", tools); + const b = injectToolsIntoPrompt("Base system.", tools); + expect(a).toBe(b); + }); + + it("deep-delivered tool objects (re-parsed YAML, server echo) stay identical", () => { + const reParsed = JSON.parse(JSON.stringify(tools)) as typeof tools; + const a = injectToolsIntoPrompt("Base system.", tools); + const b = injectToolsIntoPrompt("Base system.", reParsed); + expect(a).toBe(b); + }); + + it("changing the base prompt changes the prompt (stability is input-bound)", () => { + const a = injectToolsIntoPrompt("Base system.", tools); + const b = injectToolsIntoPrompt("Base system PLUS a change.", tools); + expect(a).not.toBe(b); + }); +}); + +// ── F1: agent-controller prefill integration ──────────────────────────────── + +describe("#121 assistant-prefill in the agent loop", () => { + function makePromptBasedProvider(): { + provider: LLMProvider; + received: LLMMessage[][]; + } { + const received: LLMMessage[][] = []; + let callIdx = 0; + const provider = { + name: "mock-prompt", + config: { + provider: "mock-prompt", + model: "test", + maxTokens: 1024, + temperature: 0.7, + }, + chat: vi.fn(async function* ( + messages: LLMMessage[], + ): AsyncGenerator { + received.push([...messages]); + const idx = callIdx++; + if (idx === 0) { + // Full XML call: the model chooses to use a tool on turn 1. + yield { + type: "text", + text: 'Let me look.\n\n\nread_file\n{"path": "a.ts"}\n', + }; + yield { type: "done", usage: { inputTokens: 10, outputTokens: 5 } }; + } else { + // Continuation of the seeded `\n` prefix: the + // backend generates only the remainder of the tag. + yield { + type: "text", + text: 'read_file\n{"path": "b.ts"}\n', + }; + yield { type: "done", usage: { inputTokens: 10, outputTokens: 4 } }; + } + }), + complete: vi.fn(), + supportsToolUse: () => false, + supportsStreaming: () => true, + countTokens: () => 10, + modelInfo: () => ({ + id: "test", + name: "Test", + provider: "mock-prompt", + contextWindow: 4096, + maxOutputTokens: 1024, + supportsToolUse: false, + supportsImages: false, + supportsStreaming: true, + }), + dispose: vi.fn(), + } as unknown as LLMProvider; + return { provider, received }; + } + + function makePromptBasedRegistry(): ToolRegistry { + const defs = [ + { + name: "read_file", + description: "Read a file", + parameters: { + type: "object", + properties: { path: { type: "string" } }, + required: ["path"], + }, + }, + ] as unknown as Parameters[0]; + return { + getAll: vi.fn().mockReturnValue(defs), + getDefinitions: vi.fn().mockReturnValue(defs), + get: vi.fn(), + register: vi.fn(), + unregister: vi.fn(), + execute: vi + .fn() + .mockResolvedValue({ success: true, output: "file content" }), + } as unknown as ToolRegistry; + } + + it("seeds the assistant prefill on tool-continuation turns only", async () => { + const { provider, received } = makePromptBasedProvider(); + const registry = makePromptBasedRegistry(); + const controller = new AgentController(provider, registry); + + await controller.processMessage("Read a.ts and b.ts"); + + expect(received.length).toBeGreaterThanOrEqual(2); + // Turn 0 — no prefill. + expect(received[0][received[0].length - 1]).not.toMatchObject({ + role: "assistant", + }); + // Turn 1 — trailing assistant seed present. + expect(received[1][received[1].length - 1]).toEqual({ + role: "assistant", + content: "\n", + }); + }); + + it("strips the unanswered seed so a plain follow-up stays clean", async () => { + const { provider, received } = makePromptBasedProvider(); + // Override: iteration 1 answers without calling a tool. + received.length = 0; + let callIdx = 0; + provider.chat = vi.fn(async function* ( + messages: LLMMessage[], + ): AsyncGenerator { + received.push([...messages]); + const idx = callIdx++; + if (idx === 0) { + yield { + type: "text", + text: 'Let me look.\n\n\nread_file\n{"path": "a.ts"}\n', + }; + yield { type: "done", usage: { inputTokens: 10, outputTokens: 5 } }; + } else { + yield { type: "text", text: "All good." }; + yield { type: "done", usage: { inputTokens: 10, outputTokens: 2 } }; + } + }) as unknown as LLMProvider["chat"]; + + const registry = makePromptBasedRegistry(); + const controller = new AgentController(provider, registry); + + const result = await controller.processMessage("Read a.ts"); + expect(result.text).toContain("All good."); + expect(result.toolCalls).toHaveLength(1); + }); +}); + +// ── F3: structured output ──────────────────────────────────────────────────── + +describe("#121 structured output", () => { + it("openai-compatible sends response_format when structuredOutput is on", async () => { + const provider = new OpenAICompatibleProvider({ + ...OAI_BASE, + structuredOutput: true, + }); + const body = await captureChatBody(provider); + expect(body.response_format).toEqual({ type: "json_object" }); + }); + + it("openai-compatible omits response_format by default", async () => { + const provider = new OpenAICompatibleProvider({ ...OAI_BASE }); + const body = await captureChatBody(provider); + expect(body.response_format).toBeUndefined(); + }); + + it("openai-compatible never constrains tool-call turns", async () => { + const provider = new OpenAICompatibleProvider({ + ...OAI_BASE, + structuredOutput: true, + }); + const body = await captureChatBody(provider, { taskHint: "toolcall" }); + expect(body.response_format).toBeUndefined(); + }); + + it("ollama sends format: json when structuredOutput is on", async () => { + const provider = new OllamaProvider({ + ...OLLAMA_BASE, + structuredOutput: true, + }); + const body = await captureChatBody(provider); + expect(body.format).toBe("json"); + }); + + it("ollama omits format: json by default", async () => { + const provider = new OllamaProvider({ ...OLLAMA_BASE }); + const body = await captureChatBody(provider); + expect(body.format).toBeUndefined(); + }); + + it("ollama keeps options.jsonFormat (explicit request wins)", async () => { + const provider = new OllamaProvider({ ...OLLAMA_BASE }); + const body = await captureChatBody(provider, { jsonFormat: true }); + expect(body.format).toBe("json"); + }); +}); + +// ── F4: KV/prompt-cache toggle ─────────────────────────────────────────────── + +describe("#121 KV-cache toggle", () => { + it("ollama sends options.cache_prompt when cachePrompt is on", async () => { + const provider = new OllamaProvider({ ...OLLAMA_BASE, cachePrompt: true }); + const body = await captureChatBody(provider); + const opts = body.options as Record; + expect(opts.cache_prompt).toBe(true); + }); + + it("ollama omits options.cache_prompt by default", async () => { + const provider = new OllamaProvider({ ...OLLAMA_BASE }); + const body = await captureChatBody(provider); + const opts = body.options as Record; + expect(opts.cache_prompt).toBeUndefined(); + }); +}); + +// ── config validation ──────────────────────────────────────────────────────── + +describe("#121 YAML validation", () => { + it("accepts structuredOutput: true on a provider", () => { + const cfg = ConfigLoader.parseYaml(` +provider: ollama +providers: + ollama: + model: m + structuredOutput: true +`); + expect(cfg.providers?.ollama?.structuredOutput).toBe(true); + }); + + it("rejects a non-boolean structuredOutput", () => { + expect(() => + ConfigLoader.parseYaml(` +provider: ollama +providers: + ollama: + structuredOutput: "yes" +`), + ).toThrow(/structuredOutput/); + }); + + it("accepts cachePrompt: false and rejects a non-boolean cachePrompt", () => { + const cfg = ConfigLoader.parseYaml(` +provider: ollama +providers: + ollama: + cachePrompt: false +`); + expect(cfg.providers?.ollama?.cachePrompt).toBe(false); + expect(() => + ConfigLoader.parseYaml(` +provider: ollama +providers: + ollama: + cachePrompt: 1 +`), + ).toThrow(/cachePrompt/); + }); +});