root 0efc7363c5 scrum 2026-04-30: 4 real fixes + 2 INFOs from cross-lineage review
3-lineage scrum (Opus 4.7 / Kimi K2.6 / Qwen3-coder) on today's wave
landed 4 real findings (2 BLOCK + 2 WARN) and 2 INFO touch-ups.
Verbatim verdicts + disposition table at:
  reports/scrum/_evidence/2026-04-30/

B-1 (BLOCK Opus + INFO Kimi convergent) — ResolveKey API:
  collapse from 3-arg (envVar, envFileName, envFilePath) to 2-arg
  (envVar, envFilePath). Pre-fix every chatd caller passed the env
  var name twice; if operator renamed *_key_env in lakehouse.toml
  while keeping the canonical KEY= line in the .env file, fallback
  silently missed.

B-2 (WARN Opus + WARN Kimi convergent) — handleProviders probe:
  drop the synthesize-then-Resolve probe; look up by name directly
  via Registry.Available(name). Prior probe synthesized "<name>/probe"
  model strings and routed through Resolve, fragile to any future
  routing rule (e.g. cloud-suffix special case).

B-3 (BLOCK Opus single — verified by trace + end-to-end probe) —
  OllamaCloud.Chat StripPrefix used "cloud" but registry routes
  "ollama_cloud/<m>". Result: upstream got the prefixed model name
  and 400'd. Smoke missed it because chatd_smoke runs without
  ollama_cloud registered. Now strips the right prefix; new
  TestOllamaCloud_StripsCorrectPrefix locks both prefix + suffix
  cases. Verified live: ollama_cloud/deepseek-v3.2 round-trips
  cleanly through the real ollama.com endpoint.

B-4 (WARN Opus single) — Ollama finishReason: read done_reason
  field instead of inferring from done bool alone. Newer Ollama
  reports done=true with done_reason="length" on truncation; the
  prior code mapped that to "stop" and lost the truncation signal
  the playbook_lift judge needs to retry. New
  TestFinishReasonFromOllama_PrefersDoneReason covers the fallback
  ladder.

INFOs:
- B-5: replace hand-rolled insertion sort in Registry.Names with
  sort.Strings (Opus called the "avoid sort import" comment a
  false economy — correct).
- A-1: clarify the playbook_lift.sh comment around -judge "" arg
  passing (Opus noted the comment said "env priority" but didn't
  reflect that the empty arg also passes through the Go driver's
  resolution chain).

False positives dismissed (3, documented in disposition.md):
- Kimi: TestMaybeDowngrade_WithConfigList wrong assertion (test IS
  correct per design — model excluded from weak list = strong = downgrade)
- Qwen: nil-deref claim (defensive code already handles nil)
- Opus: qwen3.5:latest doesn't exist on Ollama hub (true on the
  public hub but local install has it)

just verify: PASS. chatd_smoke 6/6 PASS. New regression tests:
3 (B-2, B-3, B-4 each get a focused test).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-30 00:28:08 -05:00

112 lines
3.5 KiB
Go

package chat
import (
"bufio"
"log/slog"
"os"
"strings"
"time"
)
// BuilderInput drives provider construction. Each field maps to one
// provider; empty fields mean "skip" (the registry won't have that
// provider — :cloud suffix or openrouter/* prefixes will 404 cleanly).
type BuilderInput struct {
OllamaURL string // local Ollama, no auth (typically http://localhost:11434)
OllamaCloudKey string // OLLAMA_CLOUD_KEY
OpenRouterKey string // OPENROUTER_API_KEY
OpenCodeKey string // OPENCODE_API_KEY
KimiKey string // KIMI_API_KEY
Timeout time.Duration // default 180s
}
// BuildRegistry constructs a Registry from the input. Logs which
// providers were registered (for operator confidence at boot).
func BuildRegistry(in BuilderInput) *Registry {
if in.Timeout == 0 {
in.Timeout = 180 * time.Second
}
var providers []Provider
registered := []string{}
// Local Ollama always registered if URL given (no auth needed).
if in.OllamaURL != "" {
providers = append(providers, NewOllama(in.OllamaURL, in.Timeout))
registered = append(registered, "ollama")
}
if in.OllamaCloudKey != "" {
providers = append(providers, NewOllamaCloud(in.OllamaCloudKey, in.Timeout))
registered = append(registered, "ollama_cloud")
}
if in.OpenRouterKey != "" {
providers = append(providers, NewOpenRouter(in.OpenRouterKey, in.Timeout))
registered = append(registered, "openrouter")
}
if in.OpenCodeKey != "" {
providers = append(providers, NewOpenCode(in.OpenCodeKey, in.Timeout))
registered = append(registered, "opencode")
}
if in.KimiKey != "" {
providers = append(providers, NewKimi(in.KimiKey, in.Timeout))
registered = append(registered, "kimi")
}
r := NewRegistry(providers...)
slog.Info("chat registry built", "providers", registered)
return r
}
// ResolveKey reads an API key with the priority chain:
// 1. Explicit env var (named by envVar)
// 2. .env file at envFilePath, looking for "<envVar>=<value>" line
// 3. "" if neither set
//
// Mirrors the Rust adapter's resolve_*_key() pattern. Empty key
// means the provider stays unregistered — operators see one fewer
// entry in the boot log instead of a 503 at first request.
//
// Phase 4 scrum fix B-1 (Opus + Kimi convergent): collapsed from
// 3-arg API to 2-arg. Previous version forced callers to pass the
// env var name twice (once for env-lookup, once for file-key-lookup),
// which let operator renames silently miss the file fallback. Now
// both lookups use the same canonical name. Operators who want a
// different file-key-name can use a different env var altogether.
func ResolveKey(envVar, envFilePath string) string {
if envVar != "" {
if v := strings.TrimSpace(os.Getenv(envVar)); v != "" {
return v
}
}
if envFilePath != "" && envVar != "" {
if v := readEnvFileVar(envFilePath, envVar); v != "" {
return v
}
}
return ""
}
// readEnvFileVar reads a KEY=value style env file and returns the
// value of `name`. Returns "" on any error or missing key. Stops at
// first match. No quoting/escaping — same simple shape that systemd
// EnvironmentFile= reads.
func readEnvFileVar(path, name string) string {
f, err := os.Open(path)
if err != nil {
return ""
}
defer f.Close()
scanner := bufio.NewScanner(f)
prefix := name + "="
for scanner.Scan() {
line := strings.TrimSpace(scanner.Text())
if line == "" || strings.HasPrefix(line, "#") {
continue
}
if strings.HasPrefix(line, prefix) {
return strings.Trim(strings.TrimPrefix(line, prefix), `"'`)
}
}
return ""
}