diff --git a/demo/openCodeMlx b/demo/openCodeMlx index d772370..d77a0ae 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -6,16 +6,45 @@ ############################################################ # --- User-tunable settings --- -# Model to run - comment one out, uncomment the other to switch. -MLX_MODEL="${MLX_MODEL:-mlx-community/Qwen3-Coder-30B-A3B-Instruct-8bit}" +# Model to run - comment one out, uncomment the other to switch. The serving +# backend (mlx-lm vs mlx-vlm) is auto-detected per model from its downloaded +# config.json (see detect_model_backend), so switching this line alone is +# enough even between a vision-capable model and a text-only one. +MLX_MODEL="${MLX_MODEL:-mlx-community/Qwen3.8-27B-4bit}" +#MLX_MODEL="${MLX_MODEL:-mlx-community/Qwen3.8-27B-8bit}" +#MLX_MODEL="${MLX_MODEL:-mlx-community/Qwen3-Coder-30B-A3B-Instruct-8bit}" #MLX_MODEL="${MLX_MODEL:-mlx-community/Qwen3-Coder-Next-4bit}" +# Optional speculative-decoding draft model, paired with MLX_MODEL above via +# mlx_vlm.server's --draft-model/--draft-kind flags (mlx-vlm backend only - +# see detect_model_backend/start_mlx_server_instance). This checkpoint holds +# only the MTP drafter weights, not a full model - it borrows token +# embeddings and the LM head from MLX_MODEL at runtime, so it must be +# derived from the exact same base checkpoint (same model, same quant +# level) as whatever MLX_MODEL points at above. Set to "" to disable +# speculative decoding and serve MLX_MODEL alone. Defaults to disabled: +# benchmarked via mlx_vlm.generate --verbose against MLX_MODEL above, this +# drafter only accepted ~55% of its drafted tokens, so paying for both the +# draft and target model every round was a net loss - 12.5 tokens-per-sec +# generation with the drafter enabled vs 21.3 tokens-per-sec without it. +MLX_DRAFT_MODEL="${MLX_DRAFT_MODEL:-}" + # Small/fast model used for lightweight background tasks (e.g. session title # generation) so those don't wait on the full-size model. Served by its own # second mlx_lm.server instance (see MLX_SMALL_PORT below) so opencode's # small_model role never needs Ollama. MLX_SMALL_MODEL="${MLX_SMALL_MODEL:-mlx-community/Qwen2.5-Coder-1.5B-Instruct-4bit}" +# Network interface to bind model downloads to. Empty (default) uses +# whatever the default route is. Opt in by setting this to a specific +# interface name (e.g. "en7") if your setup needs it - for example, some +# VPN clients (GlobalProtect was observed doing this) reset every connection +# to huggingface.co outright when the VPN tunnel is the default route, and +# binding to a real Ethernet/Wi-Fi interface bypasses that. If the named +# interface doesn't exist/resolve on your machine this warns and falls back +# to the default route automatically. +MLX_DOWNLOAD_INTERFACE="${MLX_DOWNLOAD_INTERFACE:-}" + # How much RAM (GB) to leave free for the OS/other apps; the rest is made # available to the GPU (iogpu.wired_limit_mb) for model weights + KV cache. # Wired GPU memory can't be reclaimed by Jetsam, so pushing this limit too @@ -126,11 +155,101 @@ ensure_mlx_lm_current() { return 0 } +# ensure_mlx_vlm_current: same not-installed/hard-error vs +# already-installed/warn-and-continue pattern as ensure_mlx_lm_current +# above, but for mlx-vlm - only needed when the selected model turns out to +# be vision-capable (see detect_model_backend). Carries the same transformers +# pin as ensure_mlx_lm_current: both packages share this one venv, so an +# unconstrained "pip install mlx-vlm" is free to upgrade transformers past +# 5.13 to satisfy its own metadata - which breaks mlx-lm's AutoTokenizer. +# register call at import time, silently taking down mlx_lm.server (still +# used unconditionally for the small background model) on the next launch. +ensure_mlx_vlm_current() { + local mode="${1:-update}" + if ! python3 -m mlx_vlm.server --help >/dev/null 2>&1; then + echo "mlx-vlm not found in venv, installing..." + if ! pip install --upgrade pip mlx-vlm "transformers>=5.7,<5.13"; then + echo "Error: mlx-vlm installation failed, cannot continue" >&2 + return 1 + fi + return 0 + fi + + if [[ "$mode" == "install-only" ]]; then + return 0 + fi + + echo "Checking for mlx-vlm updates..." + if ! pip install --upgrade pip mlx-vlm "transformers>=5.7,<5.13"; then + echo "Warning: mlx-vlm update check failed, continuing with existing installation." >&2 + fi + return 0 +} + +# _resolve_download_source_ip: resolves MLX_DOWNLOAD_INTERFACE (if set) to +# a local IP address via macOS's ipconfig, so model downloads can be bound +# to that interface (see MLX_DOWNLOAD_INTERFACE comment above). Echoes the +# IP, or nothing if unset or unresolvable (falls back to default routing). +_resolve_download_source_ip() { + [[ -z "${MLX_DOWNLOAD_INTERFACE:-}" ]] && return 0 + local ip + ip=$(ipconfig getifaddr "$MLX_DOWNLOAD_INTERFACE" 2>/dev/null) + if [[ -z "$ip" ]]; then + echo "Warning: could not resolve an IP for interface $MLX_DOWNLOAD_INTERFACE, downloading via default route." >&2 + return 0 + fi + echo "$ip" +} + +# _source_ip_bind_python_prelude : echoes python source that +# monkey-patches socket.socket so every outgoing connection binds to +# first. urllib3 (which huggingface_hub/modelscope's HTTP stack sits on) +# implements its own connection setup rather than delegating to stdlib's +# socket.create_connection, so patching that wouldn't intercept these +# requests - subclassing socket.socket itself is the reliable way to force +# every HTTP stack's outgoing connections over a specific interface. +# Echoes nothing if is empty (download proceeds via default routing). +_source_ip_bind_python_prelude() { + local ip="$1" + [[ -z "$ip" ]] && return 0 + cat < _download_via_huggingface() { local model_id="$1" local_dir="$2" echo "Downloading $model_id from Hugging Face Hub to $local_dir..." - python3 -c " + # hf_xet (huggingface_hub's fast-transfer backend, used automatically when + # the hf_xet package is installed) has its own Rust networking stack that + # bypasses the socket.socket bind below entirely - confirmed live to hang + # indefinitely (0% CPU, no progress) on this author's network when xet + # transfers can't get through, even though plain-requests API/metadata + # calls (which do honor the bind) succeed fine. Disable it whenever we're + # actually binding to a specific interface, since that binding is the + # whole point and xet silently defeats it. + local xet_env=() + if [[ -n "${DOWNLOAD_SOURCE_IP:-}" ]]; then + xet_env=(HF_HUB_DISABLE_XET=1) + fi + env "${xet_env[@]}" python3 -c " +$(_source_ip_bind_python_prelude "${DOWNLOAD_SOURCE_IP:-}") import sys try: from huggingface_hub import snapshot_download @@ -146,6 +265,7 @@ _download_via_modelscope() { local model_id="$1" local_dir="$2" echo "Downloading $model_id from ModelScope to $local_dir..." python3 -c " +$(_source_ip_bind_python_prelude "${DOWNLOAD_SOURCE_IP:-}") import sys try: from modelscope import snapshot_download @@ -166,6 +286,49 @@ _download_via() { esac } +# _quant_suffix : echoes the trailing "bit" quantization token from an MLX model +# id (e.g. "mlx-community/Qwen3.8-27B-4bit" -> "4bit", "...-4bit-DWQ" -> "4bit"), or nothing if +# the id doesn't end in one. Used to fail fast on a draft/main quant mismatch (see the +# MLX_DRAFT_MODEL check in main()) rather than silently serving a mismatched drafter. +# The optional trailing "([-_][A-Za-z]+)?" allows exactly one quantization-method qualifier +# after the bit count (DWQ, AWQ, etc.) - NOT "*" (zero-or-more), which would let an arbitrary +# multi-word tail re-match: "8bit-prefixed-model" must stay a non-match, since nothing about +# that id says its quant level is 8bit. +_quant_suffix() { + local model_id="$1" + # Deliberately always returns 0: a bare "x=$(_quant_suffix ...)" assignment under main()'s + # set -e takes the exit status of the command substitution, so a no-match model id (which + # [[ ... ]] reports as status 1) would otherwise silently kill the whole script right after + # the ~27GB model download, with no error message and no cleanup trap installed yet. + if [[ "$model_id" =~ ([0-9]+bit)([-_][A-Za-z]+)?$ ]]; then + echo "${BASH_REMATCH[1]}" + fi + return 0 +} + +# detect_model_backend : echoes "mlx_vlm" if the downloaded +# model's config.json declares a vision_config (i.e. it's a +# vision-language checkpoint that mlx_lm.server can't load - see +# https://github.com/ml-explore/mlx-lm and mlx-vlm's own architecture +# registration), else "mlx_lm". Defaults to "mlx_lm" on any read/parse +# failure so existing text-only models keep behaving exactly as before. +detect_model_backend() { + local model_dir="$1" + if python3 -c " +import json, sys +try: + with open('$model_dir/config.json') as f: + cfg = json.load(f) +except Exception: + sys.exit(1) +sys.exit(0 if 'vision_config' in cfg else 1) +" 2>/dev/null; then + echo "mlx_vlm" + else + echo "mlx_lm" + fi +} + # sync_model # Ensures holds a current copy of , trying # then (each huggingface|modelscope). If doesn't @@ -258,7 +421,7 @@ main() { MLX_SMALL_PROMPT_CACHE_BYTES="${MLX_SMALL_PROMPT_CACHE_BYTES:-$(( GPU_WIRED_LIMIT * 1024 * 1024 / 10 ))}" if [[ $RAM_GB -lt 64 ]]; then - echo "Warning: $MLX_MODEL may need ~45GB for weights + KV cache and may not fit" + echo "Warning: $MLX_MODEL may need ~25GB for weights + KV cache and may not fit" echo "in ${RAM_GB}GB RAM. Override with a smaller/lower-bit model via the" echo "MLX_MODEL environment variable if you hit an OOM." fi @@ -333,6 +496,8 @@ main() { fi fi + DOWNLOAD_SOURCE_IP=$(_resolve_download_source_ip) + LOCAL_MODEL_DIR="$MLX_DIR/models/$(echo "$MLX_MODEL" | tr '/' '--')" # Defaults to ModelScope-first: HF is blocked in this author's environment, @@ -348,6 +513,40 @@ main() { exit 1 fi + MAIN_MODEL_BACKEND=$(detect_model_backend "$LOCAL_MODEL_DIR") + if [[ "$MAIN_MODEL_BACKEND" == "mlx_vlm" ]]; then + echo "$MLX_MODEL is a vision-language checkpoint, serving it via mlx-vlm instead of mlx-lm..." + if ! ensure_mlx_vlm_current "$ensure_mode"; then + exit 1 + fi + fi + + # Speculative decoding is only wired up on the mlx_vlm path below (--draft-model is never + # passed to plain mlx_lm.server) - downloading a multi-GB drafter for a model served via + # mlx_lm would just be discarded, so gate on the detected backend too, not MLX_DRAFT_MODEL + # alone. Also fail fast on a quant mismatch: the drafter borrows token embeddings and the LM + # head from MLX_MODEL at runtime, so it must share the exact same base checkpoint and quant + # level, and nothing downstream would catch a mismatch on its own. + LOCAL_DRAFT_MODEL_DIR="" + if [[ -n "${MLX_DRAFT_MODEL:-}" && "$MAIN_MODEL_BACKEND" == "mlx_vlm" ]]; then + main_quant=$(_quant_suffix "$MLX_MODEL") + draft_quant=$(_quant_suffix "$MLX_DRAFT_MODEL") + if [[ -n "$main_quant" && -n "$draft_quant" && "$main_quant" != "$draft_quant" ]]; then + echo "Error: MLX_DRAFT_MODEL ($MLX_DRAFT_MODEL, quant $draft_quant) does not match" >&2 + echo "MLX_MODEL's quant ($MLX_MODEL, quant $main_quant). The drafter must be derived" >&2 + echo "from the exact same base checkpoint and quant level as MLX_MODEL. Disable" >&2 + echo "speculative decoding (MLX_DRAFT_MODEL=\"\") or pick a matching drafter." >&2 + exit 1 + fi + LOCAL_DRAFT_MODEL_DIR="$MLX_DIR/models/$(echo "$MLX_DRAFT_MODEL" | tr '/' '-')" + if ! sync_model "$MLX_DRAFT_MODEL" "$LOCAL_DRAFT_MODEL_DIR" "$PRIMARY" "$FALLBACK" "$ensure_mode"; then + exit 1 + fi + elif [[ -n "${MLX_DRAFT_MODEL:-}" ]]; then + echo "Warning: MLX_DRAFT_MODEL is set but $MLX_MODEL is served via mlx_lm; speculative" >&2 + echo "decoding here is only wired up for the mlx_vlm backend. Ignoring MLX_DRAFT_MODEL." >&2 + fi + LOCAL_SMALL_MODEL_DIR="$MLX_DIR/models/$(echo "$MLX_SMALL_MODEL" | tr '/' '--')" if ! sync_model "$MLX_SMALL_MODEL" "$LOCAL_SMALL_MODEL_DIR" "$PRIMARY" "$FALLBACK" "$ensure_mode"; then @@ -388,16 +587,37 @@ EOF } start_mlx_server_instance() { - local label="$1" model_dir="$2" port="$3" log_file="$4" pid_var_name="$5" model_id="$6" model_env_var="$7" prefill_step_size="$8" prompt_cache_bytes="$9" - - echo "Starting $label with model $model_id on port $port..." - mlx_lm.server \ - --model "$model_dir" \ - --host 127.0.0.1 \ - --port "$port" \ - --prefill-step-size "$prefill_step_size" \ - --prompt-cache-bytes "$prompt_cache_bytes" \ - >> "$log_file" 2>&1 & + local label="$1" model_dir="$2" port="$3" log_file="$4" pid_var_name="$5" model_id="$6" model_env_var="$7" prefill_step_size="$8" cache_limit="$9" backend="${10:-mlx_lm}" draft_model_dir="${11:-}" + + echo "Starting $label with model $model_id on port $port (backend: $backend)..." + if [[ "$backend" == "mlx_vlm" ]]; then + # mlx-vlm has no --prompt-cache-bytes equivalent; --max-kv-size (token + # count, reusing the same context-limit knob) is its closest analogue + # for bounding KV-cache growth per the OOM-guard rationale above. + # --trust-remote-code included per observed mlx-vlm usage for this + # architecture family. + local draft_args=() + if [[ -n "$draft_model_dir" ]]; then + draft_args=(--draft-model "$draft_model_dir" --draft-kind mtp) + fi + python3 -m mlx_vlm.server \ + --model "$model_dir" \ + --host 127.0.0.1 \ + --port "$port" \ + --prefill-step-size "$prefill_step_size" \ + --max-kv-size "$cache_limit" \ + --trust-remote-code \ + "${draft_args[@]}" \ + >> "$log_file" 2>&1 & + else + mlx_lm.server \ + --model "$model_dir" \ + --host 127.0.0.1 \ + --port "$port" \ + --prefill-step-size "$prefill_step_size" \ + --prompt-cache-bytes "$cache_limit" \ + >> "$log_file" 2>&1 & + fi printf -v "$pid_var_name" '%s' "$!" echo "Waiting for $label to come up (first run may need to load a large model into memory, this can take a while - Ctrl+C to abort)..." @@ -455,8 +675,10 @@ EOF kill -9 "$pid" 2>/dev/null fi } + local main_server_label="mlx_lm.server (main)" + [[ "$MAIN_MODEL_BACKEND" == "mlx_vlm" ]] && main_server_label="mlx_vlm.server (main)" stop_and_report "mlx_lm.server (small)" "${SMALL_SERVER_PID:-}" & - stop_and_report "mlx_lm.server (main)" "${SERVER_PID:-}" & + stop_and_report "$main_server_label" "${SERVER_PID:-}" & wait # Reap here, in this shell (the servers' actual parent) - by now each @@ -471,11 +693,24 @@ EOF # server process last time. trap cleanup EXIT INT TERM HUP - if ! start_mlx_server_instance "mlx_lm.server (main)" "$LOCAL_MODEL_DIR" "$MLX_PORT" "$MLX_DIR/server.log" "SERVER_PID" "$MLX_MODEL" "MLX_MODEL" "$MLX_PREFILL_STEP_SIZE" "$MLX_PROMPT_CACHE_BYTES"; then + if [[ "$MAIN_MODEL_BACKEND" == "mlx_vlm" ]]; then + # mlx-vlm has no --prompt-cache-bytes equivalent; --max-kv-size takes a token count, not + # bytes, so it can't reuse MLX_PROMPT_CACHE_BYTES directly the way the mlx_lm branch does. + # Reusing MLX_CONTEXT_LIMIT (128k tokens by default) as a stand-in is unverified and may not + # actually bind the same GPU-memory budget the kernel-panic guard above was sized around - + # this model's real per-token KV footprint hasn't been measured. Override with + # MLX_VLM_MAX_KV_SIZE once that's been profiled (e.g. watch iogpu.wired_limit_mb headroom + # while serving a large prompt) rather than trusting this default for a memory-safety guard. + MAIN_CACHE_LIMIT="${MLX_VLM_MAX_KV_SIZE:-$MLX_CONTEXT_LIMIT}" + else + MAIN_CACHE_LIMIT="$MLX_PROMPT_CACHE_BYTES" + fi + + if ! start_mlx_server_instance "main model server" "$LOCAL_MODEL_DIR" "$MLX_PORT" "$MLX_DIR/server.log" "SERVER_PID" "$MLX_MODEL" "MLX_MODEL" "$MLX_PREFILL_STEP_SIZE" "$MAIN_CACHE_LIMIT" "$MAIN_MODEL_BACKEND" "$LOCAL_DRAFT_MODEL_DIR"; then exit 1 fi - if ! start_mlx_server_instance "mlx_lm.server (small)" "$LOCAL_SMALL_MODEL_DIR" "$MLX_SMALL_PORT" "$MLX_DIR/small_server.log" "SMALL_SERVER_PID" "$MLX_SMALL_MODEL" "MLX_SMALL_MODEL" "$MLX_SMALL_PREFILL_STEP_SIZE" "$MLX_SMALL_PROMPT_CACHE_BYTES"; then + if ! start_mlx_server_instance "small model server" "$LOCAL_SMALL_MODEL_DIR" "$MLX_SMALL_PORT" "$MLX_DIR/small_server.log" "SMALL_SERVER_PID" "$MLX_SMALL_MODEL" "MLX_SMALL_MODEL" "$MLX_SMALL_PREFILL_STEP_SIZE" "$MLX_SMALL_PROMPT_CACHE_BYTES" "mlx_lm"; then exit 1 fi diff --git a/demo/test_quant_suffix.sh b/demo/test_quant_suffix.sh new file mode 100755 index 0000000..2461b64 --- /dev/null +++ b/demo/test_quant_suffix.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +set -u +cd "$(dirname "${BASH_SOURCE[0]}")" +source ./test_helpers.sh +source ./openCodeMlx + +check "extracts a 4bit suffix" "4bit" "$(_quant_suffix 'mlx-community/Qwen3.8-27B-4bit')" +check "extracts an 8bit suffix" "8bit" "$(_quant_suffix 'mlx-community/Qwen3.8-27B-8bit')" +check "extracts a multi-digit bit suffix" "16bit" "$(_quant_suffix 'mlx-community/Some-Model-16bit')" +check "empty for a model id with no quant suffix" "" "$(_quant_suffix 'mlx-community/Qwen3-Coder-Next')" +check "does not match a mid-string bit token" "" "$(_quant_suffix 'mlx-community/8bit-prefixed-model')" +check "extracts the bit count from a DWQ-qualified id" "4bit" "$(_quant_suffix 'mlx-community/Qwen3.8-27B-4bit-DWQ')" +check "extracts the bit count from an underscore-qualified id" "8bit" "$(_quant_suffix 'mlx-community/Qwen3.8-27B-8bit_dwq')" +check "does not treat a multi-word non-quant tail as a qualifier" "" "$(_quant_suffix 'mlx-community/8bit-prefixed-model-name')" + +# Regression guard: main() runs under `set -e`, and its call sites assign the result via bare +# "main_quant=$(_quant_suffix "$MLX_MODEL")" - not inside an if/while condition, so under set -e +# the assignment's exit status must never be non-zero, or a no-match model id (any checkpoint +# id that doesn't end in "bit") would silently kill the whole script right after the +# multi-GB model download, with no error message and no cleanup trap installed yet. +_quant_suffix 'mlx-community/Qwen3.8-27B-4bit' >/dev/null +check "exits 0 on a matching model id" "0" "$?" +_quant_suffix 'mlx-community/Qwen3-Coder-Next' >/dev/null +check "exits 0 (not 1) on a non-matching model id, so set -e callers survive" "0" "$?" +check "a bare assignment under set -e still reaches the next line" "REACHED" \ + "$(set -e; no_suffix_quant=$(_quant_suffix 'mlx-community/Qwen3-Coder-Next'); echo "REACHED")" + +report diff --git a/demo/test_updates_mlxlm.sh b/demo/test_updates_mlxlm.sh index 36b8748..47a9340 100755 --- a/demo/test_updates_mlxlm.sh +++ b/demo/test_updates_mlxlm.sh @@ -15,7 +15,12 @@ REAL_PATH="/usr/bin:/bin:/usr/local/bin" bin_a="$work/a"; mkdir -p "$bin_a" log_a="$work/a.log" make_stub "$bin_a" pip 0 "$log_a" -# no mlx_lm.server stub -> `command -v`/direct exec fails, simulating "not installed" +# Explicitly stub mlx_lm.server to fail ensure_mlx_lm_current's "mlx_lm.server --help" +# check, rather than relying on it being absent from $REAL_PATH: "not installed" is a +# property of this test's stub, not of whatever the machine running it happens to have +# on PATH. A machine with a real mlx_lm.server on /usr/local/bin would otherwise +# silently take the "already installed" branch instead, making this scenario a no-op. +make_stub "$bin_a" mlx_lm.server 1 "$log_a" ( PATH="$bin_a:$REAL_PATH" ensure_mlx_lm_current >/dev/null 2>&1 ) @@ -26,6 +31,7 @@ check "not-installed + pip install succeeds -> pip was invoked with --upgrade" " bin_b="$work/b"; mkdir -p "$bin_b" log_b="$work/b.log" make_stub "$bin_b" pip 1 "$log_b" +make_stub "$bin_b" mlx_lm.server 1 "$log_b" # force "not installed", see scenario A ( PATH="$bin_b:$REAL_PATH" ensure_mlx_lm_current >/dev/null 2>&1 ) diff --git a/demo/test_updates_mlxvlm.sh b/demo/test_updates_mlxvlm.sh new file mode 100755 index 0000000..bf0effb --- /dev/null +++ b/demo/test_updates_mlxvlm.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +set -u +cd "$(dirname "${BASH_SOURCE[0]}")" +source ./test_helpers.sh +source ./openCodeMlx + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT +# Stub dirs are prepended to a real system PATH, not used exclusively: +# ensure_mlx_vlm_current itself shells out to other real commands, which an +# exclusive stub-only PATH would hide too. +REAL_PATH="/usr/bin:/bin:/usr/local/bin" + +# Scenario A: mlx-vlm missing, pip install stub succeeds -> returns 0, and pip is invoked with +# the same transformers pin ensure_mlx_lm_current carries. This is the actual fix under test: +# both packages share one venv, so an unconstrained "pip install mlx-vlm" is free to upgrade +# transformers past the version mlx-lm's AutoTokenizer.register call tolerates, silently +# breaking mlx_lm.server (still used unconditionally for the small background model). +bin_a="$work/a"; mkdir -p "$bin_a" +log_a="$work/a.log" +make_stub "$bin_a" pip 0 "$log_a" +# Explicitly stub python3 to fail ensure_mlx_vlm_current's "python3 -m mlx_vlm.server +# --help" check, rather than relying on the real system python3 lacking mlx_vlm: "not +# installed" is a property of this test's stub, not of whatever python3 environment the +# machine running it happens to have. A machine where mlx_vlm is importable from the +# default python3 (e.g. a shared/activated venv) would otherwise silently take the +# "already installed" branch instead, making this scenario a no-op. +make_stub "$bin_a" python3 1 "$log_a" +( + PATH="$bin_a:$REAL_PATH" ensure_mlx_vlm_current >/dev/null 2>&1 +) +check "not-installed + pip install succeeds -> returns 0" "0" "$?" +check "not-installed -> pip carries the shared transformers pin" "1" "$(grep -c 'transformers>=5.7,<5.13' "$log_a")" + +# Scenario B: mlx-vlm missing, pip install stub fails -> hard error (returns 1). +bin_b="$work/b"; mkdir -p "$bin_b" +log_b="$work/b.log" +make_stub "$bin_b" pip 1 "$log_b" +make_stub "$bin_b" python3 1 "$log_b" # force "not installed", see scenario A +( + PATH="$bin_b:$REAL_PATH" ensure_mlx_vlm_current >/dev/null 2>&1 +) +check "not-installed + pip install fails -> returns 1 (hard error)" "1" "$?" + +# Scenario C: mlx-vlm present, pip install stub fails -> still returns 0 (warn+continue). +bin_c="$work/c"; mkdir -p "$bin_c" +log_c="$work/c.log" +make_stub "$bin_c" python3 0 "$log_c" +make_stub "$bin_c" pip 1 "$log_c" +( + PATH="$bin_c:$REAL_PATH" ensure_mlx_vlm_current >/dev/null 2>&1 +) +check "already-installed + pip update fails -> still returns 0 (warn and continue)" "0" "$?" + +# Scenario D: mlx-vlm present, pip install stub succeeds -> pip was still invoked (not skipped) +# and still carries the transformers pin. +bin_d="$work/d"; mkdir -p "$bin_d" +log_d="$work/d.log" +make_stub "$bin_d" python3 0 "$log_d" +make_stub "$bin_d" pip 0 "$log_d" +( + PATH="$bin_d:$REAL_PATH" ensure_mlx_vlm_current >/dev/null 2>&1 +) +check "already-installed + pip update succeeds -> returns 0" "0" "$?" +check "already-installed -> pip is still invoked (update not skipped)" "1" "$(grep -c '^pip ' "$log_d")" +check "already-installed -> pip update still carries the shared transformers pin" "1" "$(grep -c 'transformers>=5.7,<5.13' "$log_d")" + +# Scenario E: mlx-vlm present, called in install-only mode -> pip upgrade must NOT be attempted. +bin_e="$work/e"; mkdir -p "$bin_e" +log_e="$work/e.log" +make_stub "$bin_e" python3 0 "$log_e" +make_stub "$bin_e" pip 1 "$log_e" +( + PATH="$bin_e:$REAL_PATH" ensure_mlx_vlm_current install-only >/dev/null 2>&1 +) +check "already-installed + install-only mode -> returns 0" "0" "$?" +check "already-installed + install-only mode -> pip was NOT invoked" "0" "$(grep -c '^pip ' "$log_e")" + +report diff --git a/demo/test_updates_opencode.sh b/demo/test_updates_opencode.sh index 978e52c..9bcba02 100755 --- a/demo/test_updates_opencode.sh +++ b/demo/test_updates_opencode.sh @@ -21,7 +21,7 @@ set -e source ./test_helpers.sh source ./openCodeMlx _install_opencode_via_curl() { return 1; } -PATH="/usr/bin:/bin:/usr/local/bin" HOME="$1" ensure_opencode_current 2>&1 +PATH="/usr/bin:/bin" HOME="$1" ensure_opencode_current 2>&1 TESTEOF # Run it; with the bug (bare call), set -e aborts before printing error. @@ -48,30 +48,55 @@ trap 'rm -rf "$work"' EXIT # Step 3), which tests override directly as a bash function instead. REAL_PATH="/usr/bin:/bin:/usr/local/bin" -# Scenario A: opencode not on PATH; the install override actually creates a -# working $HOME/.opencode/bin/opencode (simulating a real successful install). -home_a="$work/a_home" -_install_opencode_via_curl() { - mkdir -p "$home_a/.opencode/bin" - cat > "$home_a/.opencode/bin/opencode" <<'INNER' +# Scenarios A/B/E need a PATH where opencode is NOT resolvable, so that +# ensure_opencode_current takes its "not found -> install" branch, but where coreutils +# (mkdir/cat/chmod, used by the install-simulation overrides above) still are. Deliberately +# excludes /usr/local/bin, unlike REAL_PATH above: it's the one realistic place a +# system-wide `opencode` might actually be symlinked to on this project's target platform +# (macOS/Homebrew) - the official installer puts it in ~/.opencode/bin, which was never on +# REAL_PATH to begin with. /usr/bin and /bin alone cover the coreutils these scenarios need. +NOT_INSTALLED_PATH="/usr/bin:/bin" + +# Guarded rather than assumed outright: even /usr/bin:/bin could theoretically contain a +# real `opencode` on some machine this hasn't been tested against. If so, skip (not fail) +# rather than let these scenarios silently exercise the "already installed -> opencode +# upgrade" branch instead - a REAL network-touching command against whatever opencode +# install actually exists, run under a test that thinks it's simulating "not installed". +if PATH="$NOT_INSTALLED_PATH" command -v opencode >/dev/null 2>&1; then + echo "SKIP: a real 'opencode' is resolvable on PATH=$NOT_INSTALLED_PATH in this" >&2 + echo "environment - scenarios A, B and E cannot safely simulate 'not installed' here" >&2 + echo "(would run a real 'opencode upgrade' instead). Skipping those three checks." >&2 + NOT_INSTALLED_SCENARIOS_SAFE=0 +else + NOT_INSTALLED_SCENARIOS_SAFE=1 +fi + +if [[ "$NOT_INSTALLED_SCENARIOS_SAFE" -eq 1 ]]; then + # Scenario A: opencode not on PATH; the install override actually creates a + # working $HOME/.opencode/bin/opencode (simulating a real successful install). + home_a="$work/a_home" + _install_opencode_via_curl() { + mkdir -p "$home_a/.opencode/bin" + cat > "$home_a/.opencode/bin/opencode" <<'INNER' #!/usr/bin/env bash exit 0 INNER - chmod +x "$home_a/.opencode/bin/opencode" -} -( - PATH="$REAL_PATH" HOME="$home_a" ensure_opencode_current >/dev/null 2>&1 -) -check "not-installed + install produces a binary -> returns 0" "0" "$?" + chmod +x "$home_a/.opencode/bin/opencode" + } + ( + PATH="$NOT_INSTALLED_PATH" HOME="$home_a" ensure_opencode_current >/dev/null 2>&1 + ) + check "not-installed + install produces a binary -> returns 0" "0" "$?" -# Scenario B: opencode not on PATH; the install override is a no-op -# (simulates a failed install -- opencode still isn't findable afterward). -home_b="$work/b_home" -_install_opencode_via_curl() { :; } -( - PATH="$REAL_PATH" HOME="$home_b" ensure_opencode_current >/dev/null 2>&1 -) -check "not-installed + install produces no binary -> returns 1 (hard error)" "1" "$?" + # Scenario B: opencode not on PATH; the install override is a no-op + # (simulates a failed install -- opencode still isn't findable afterward). + home_b="$work/b_home" + _install_opencode_via_curl() { :; } + ( + PATH="$NOT_INSTALLED_PATH" HOME="$home_b" ensure_opencode_current >/dev/null 2>&1 + ) + check "not-installed + install produces no binary -> returns 1 (hard error)" "1" "$?" +fi # Scenario C: opencode already on PATH, `opencode upgrade` stub succeeds. bin_c="$work/c"; mkdir -p "$bin_c" @@ -108,8 +133,12 @@ check "already-installed + install-only mode -> upgrade was NOT invoked" "0" "$( # Scenario E: opencode not on PATH, install fails (returns 1), ensure_opencode_current # is called under `set -e`. Verify that the function's return 1 is reached (not aborted # by set -e when _install_opencode_via_curl exits with status 1). The function should -# return 1 to the outer subshell (not die uncontrolled). -test_set_e_with_failed_install -check "install fails under set -e -> function returns 1 (not aborted by set -e)" "0" "$?" +# return 1 to the outer subshell (not die uncontrolled). Same NOT_INSTALLED_PATH +# assumption as scenarios A/B (test_set_e_with_failed_install hardcodes the same +# "/usr/bin:/bin" internally) - guarded above, so only run this when that guard held. +if [[ "$NOT_INSTALLED_SCENARIOS_SAFE" -eq 1 ]]; then + test_set_e_with_failed_install + check "install fails under set -e -> function returns 1 (not aborted by set -e)" "0" "$?" +fi report diff --git a/models.sh b/models.sh index 0bfd7f9..6f268ef 100755 --- a/models.sh +++ b/models.sh @@ -1,5 +1,45 @@ #!/bin/sh +force=false +while [ $# -gt 0 ]; do + case "$1" in + -f|--force) + force=true + ;; + *) + echo "Unknown option: $1" + echo "Usage: $0 [-f|--force]" + exit 1 + ;; + esac + shift +done + +# Model names/contexts are not duplicated here: src/main/bin/lca is the canonical +# source (it must be self-contained since it's distributed standalone), so we read +# its named variables via a targeted grep+eval. +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +LCA_SCRIPT="$SCRIPT_DIR/src/main/bin/lca" +if [ ! -f "$LCA_SCRIPT" ]; then + echo "Error: canonical model config not found at $LCA_SCRIPT" >&2 + exit 1 +fi +eval "$(grep -E '^(BASE_CHAT_MODEL|BASE_FALLBACK_MODEL|EMBEDDING_MODEL|CUSTOM_CHAT_MODEL|CUSTOM_CHAT_CONTEXT|QWEN_EXTRA_PARAMS|CUSTOM_FALLBACK_MODEL|CUSTOM_FALLBACK_CONTEXT|REVIEW_MODEL|REVIEW_CONTEXT|DEFAULT_CONTEXT_WINDOW|MODEL_STATE_DIR)=' "$LCA_SCRIPT")" + +# Guard against a variable being renamed/added in lca without updating the grep alternation +# above (which would silently eval to empty) or a value in lca referencing another variable +# defined later in that file (which would also evaluate to empty here). QWEN_EXTRA_PARAMS is +# deliberately excluded: an empty extra-params string is a legitimate value, not a bug. +for _v in BASE_CHAT_MODEL BASE_FALLBACK_MODEL EMBEDDING_MODEL CUSTOM_CHAT_MODEL \ + CUSTOM_CHAT_CONTEXT CUSTOM_FALLBACK_MODEL CUSTOM_FALLBACK_CONTEXT REVIEW_MODEL \ + REVIEW_CONTEXT DEFAULT_CONTEXT_WINDOW MODEL_STATE_DIR; do + eval "_val=\$$_v" + if [ -z "$_val" ]; then + echo "Error: $_v not resolved from $LCA_SCRIPT" >&2 + exit 1 + fi +done + os="" case "$(uname -s)" in Darwin) @@ -41,6 +81,11 @@ if ! command -v ollama >/dev/null 2>&1; then esac fi +get_model_id() { + model="$1" + ollama list 2>/dev/null | awk -v m="$model" '$1 == m {print $2; exit}' +} + checkAndInstall() { model="$1" echo "Checking for $model model..." @@ -57,52 +102,78 @@ createCustomModel() { base_model="$1" custom_name="$2" context_size="$3" + extra_params="${4:-}" + + current_id="$(get_model_id "$base_model")" + if [ -z "$current_id" ]; then + echo "Warning: could not retrieve ID for $base_model. Skipping $custom_name." + return + fi - echo "Creating custom model $custom_name from $base_model with context size $context_size..." + # Mirrors src/main/bin/lca's rebuild_custom_model_if_changed: fingerprint the full desired + # Modelfile recipe (base id + context + extra params), not just the base model's id, so a + # context/parameter-only change is detected even when the base model itself hasn't changed. + # Shares lca's MODEL_STATE_DIR so the two scripts agree on whether a custom model is stale. + desired_signature="${current_id}|${context_size}|${extra_params}" + state_file="${MODEL_STATE_DIR}/${custom_name}.id" + saved_signature="" + if [ -f "$state_file" ]; then + saved_signature="$(cat "$state_file")" + fi - # Check if custom model already exists - if ollama list 2>/dev/null | grep -q "^$custom_name"; then - echo "$custom_name already exists." + custom_exists="no" + if ollama list 2>/dev/null | awk '{print $1}' | grep -Fxq "${custom_name}:latest"; then + custom_exists="yes" + fi + + if [ "$force" != true ] && [ "$desired_signature" = "$saved_signature" ] && [ "$custom_exists" = "yes" ]; then + echo "$custom_name is up to date." return fi + if [ "$custom_exists" = "yes" ]; then + echo "Rebuilding $custom_name (base model, context, or parameters changed; or --force)..." + ollama rm "$custom_name" + else + echo "$custom_name not found. Creating..." + fi + # Create a temporary Modelfile modelfile=$(mktemp) - cat > "$modelfile" << EOF -FROM $base_model -PARAMETER num_ctx $context_size -EOF + { + echo "FROM $base_model" + echo "PARAMETER num_ctx $context_size" + if [ -n "$extra_params" ]; then + old_ifs="$IFS" + IFS=';' + for kv in $extra_params; do + key="${kv%%=*}" + value="${kv#*=}" + echo "PARAMETER $key $value" + done + IFS="$old_ifs" + fi + } > "$modelfile" # Create the custom model ollama create "$custom_name" -f "$modelfile" # Clean up rm "$modelfile" + mkdir -p "$MODEL_STATE_DIR" + printf '%s\n' "$desired_signature" > "$state_file" echo "$custom_name created successfully." } # Install base models -#checkAndInstall deepseek-coder:6.7b -checkAndInstall qwen3.6:35b-a3b -checkAndInstall gpt-oss:20b -checkAndInstall nomic-embed-text:latest +checkAndInstall "$BASE_CHAT_MODEL" +checkAndInstall "$BASE_FALLBACK_MODEL" +checkAndInstall "$EMBEDDING_MODEL" -# Create custom models with larger context (128k=131072, 64k=65536) -createCustomModel qwen3.6:35b-a3b qwen3.6-128k 131072 -createCustomModel gpt-oss:20b gpt-oss-64k 65536 +# Create custom models with larger context +createCustomModel "$BASE_CHAT_MODEL" "$CUSTOM_CHAT_MODEL" "$CUSTOM_CHAT_CONTEXT" "$QWEN_EXTRA_PARAMS" +createCustomModel "$BASE_FALLBACK_MODEL" "$CUSTOM_FALLBACK_MODEL" "$CUSTOM_FALLBACK_CONTEXT" # Create review model with thinking disabled and smaller context for faster response -echo "Creating review model qwen3.6-review from qwen3.6:35b-a3b..." -if ollama list 2>/dev/null | grep -q "^qwen3.6-review"; then - echo "qwen3.6-review already exists." -else - modelfile=$(mktemp) - cat > "$modelfile" << EOF -FROM qwen3.6:35b-a3b -PARAMETER num_ctx 65536 -EOF - ollama create qwen3.6-review -f "$modelfile" - rm "$modelfile" - echo "qwen3.6-review created successfully." -fi \ No newline at end of file +createCustomModel "$BASE_CHAT_MODEL" "$REVIEW_MODEL" "$REVIEW_CONTEXT" "$QWEN_EXTRA_PARAMS" \ No newline at end of file diff --git a/src/main/bin/lca b/src/main/bin/lca index 29a23ac..b66d2fd 100755 --- a/src/main/bin/lca +++ b/src/main/bin/lca @@ -6,8 +6,26 @@ LIB_DIR="${HOME}/.local/lib" JAR_PREFIX="local-coding-assistant" JAR_PATTERN="${JAR_PREFIX}-*-exec.jar" API_URL="https://api.github.com/repos/${REPO}/releases/latest" -BASE_MODELS="qwen3.6:35b-a3b gpt-oss:20b" -CUSTOM_MODELS="qwen3.6:35b-a3b|qwen3.6-128k|131072 gpt-oss:20b|gpt-oss-64k|65536" +# Canonical model configuration. models.sh derives its own model list from these +# variables (via a targeted grep+eval), so this file is the single place to edit +# when changing a model name or context size. +BASE_CHAT_MODEL="qwen3.8:27b" +BASE_FALLBACK_MODEL="gpt-oss:20b" +EMBEDDING_MODEL="nomic-embed-text:latest" +CUSTOM_CHAT_MODEL="qwen3.8-192k" +CUSTOM_CHAT_CONTEXT="196608" +CUSTOM_FALLBACK_MODEL="gpt-oss-64k" +CUSTOM_FALLBACK_CONTEXT="65536" +REVIEW_MODEL="qwen3.8-review" +REVIEW_CONTEXT="131072" +# Extra Modelfile PARAMETER lines for the qwen3.8-based custom models (chat + review; +# NOT the gpt-oss fallback), as "key=value" pairs separated by ";" (num_batch matches +# llama.cpp -b; num_gpu 99 forces full GPU offload, since llama.cpp's own layer/VRAM +# auto-detection is otherwise more conservative than the mlx backend it replaced). +QWEN_EXTRA_PARAMS="num_batch=2048;num_gpu=99" +DEFAULT_CONTEXT_WINDOW="131072" +BASE_MODELS="$BASE_CHAT_MODEL $BASE_FALLBACK_MODEL $EMBEDDING_MODEL" +CUSTOM_MODELS="$BASE_CHAT_MODEL|$CUSTOM_CHAT_MODEL|$CUSTOM_CHAT_CONTEXT|$QWEN_EXTRA_PARAMS $BASE_FALLBACK_MODEL|$CUSTOM_FALLBACK_MODEL|$CUSTOM_FALLBACK_CONTEXT $BASE_CHAT_MODEL|$REVIEW_MODEL|$REVIEW_CONTEXT|$QWEN_EXTRA_PARAMS" MODEL_STATE_DIR="${HOME}/.lca/model_state" die() { @@ -96,48 +114,47 @@ get_model_id() { ollama list 2>/dev/null | awk -v m="$model" '$1 == m {print $2; exit}' } -ensure_custom_model() { +write_modelfile() { local base="$1" - local custom="$2" - local ctx="$3" - local list_output - list_output="$(ollama list 2>/dev/null || true)" - if printf '%s\n' "$list_output" | awk '{print $1}' | grep -Fxq "$custom:latest"; then - echo "${custom} custom model already exists." - return + local ctx="$2" + local extra="${3:-}" + local old_ifs + local kv + local key + local value + printf 'FROM %s\n' "$base" + printf 'PARAMETER num_ctx %s\n' "$ctx" + if [ -n "$extra" ]; then + old_ifs="$IFS" + IFS=';' + for kv in $extra; do + key="${kv%%=*}" + value="${kv#*=}" + printf 'PARAMETER %s %s\n' "$key" "$value" + done + IFS="$old_ifs" fi - echo "Creating custom model ${custom} from ${base} with context ${ctx}..." - local modelfile - modelfile="$(mktemp)" - cat > "$modelfile" < "${MODEL_STATE_DIR}/${custom}.id" - fi - echo "${custom} created successfully." } rebuild_custom_model_if_changed() { local base="$1" local custom="$2" local ctx="$3" + local extra="${4:-}" local current_id current_id="$(get_model_id "$base")" if [ -z "$current_id" ]; then echo "Warning: could not retrieve ID for ${base}." return fi - local saved_id="" + # The signature covers everything that goes into the Modelfile - not just the base + # model's id - so a context-size or PARAMETER change is detected even when the base + # model itself hasn't changed (a base-id-only signature would silently miss that). + local desired_signature="${current_id}|${ctx}|${extra}" + local saved_signature="" local state_file="${MODEL_STATE_DIR}/${custom}.id" if [ -f "$state_file" ]; then - saved_id="$(cat "$state_file")" + saved_signature="$(cat "$state_file")" fi local list_output list_output="$(ollama list 2>/dev/null || true)" @@ -145,18 +162,15 @@ rebuild_custom_model_if_changed() { if printf '%s\n' "$list_output" | awk '{print $1}' | grep -Fxq "$custom:latest"; then custom_exists="yes" fi - if [ "$current_id" != "$saved_id" ] || [ "$custom_exists" = "no" ]; then - echo "Rebuilding ${custom} (base model changed or custom model missing)..." + if [ "$desired_signature" != "$saved_signature" ] || [ "$custom_exists" = "no" ]; then + echo "Rebuilding ${custom} (base model, context, or parameters changed; or custom model missing)..." local modelfile modelfile="$(mktemp)" - cat > "$modelfile" < "$modelfile" ollama create "$custom" -f "$modelfile" rm -f "$modelfile" mkdir -p "$MODEL_STATE_DIR" - printf '%s\n' "$current_id" > "$state_file" + printf '%s\n' "$desired_signature" > "$state_file" echo "${custom} rebuilt successfully." else echo "${custom} is up to date." @@ -177,29 +191,9 @@ ensure_prerequisites() { base="$(printf '%s\n' "$entry" | cut -d'|' -f1)" custom="$(printf '%s\n' "$entry" | cut -d'|' -f2)" ctx="$(printf '%s\n' "$entry" | cut -d'|' -f3)" - ensure_custom_model "$base" "$custom" "$ctx" + extra="$(printf '%s\n' "$entry" | cut -d'|' -f4)" + rebuild_custom_model_if_changed "$base" "$custom" "$ctx" "$extra" done - ensure_review_model -} - -ensure_review_model() { - local custom="qwen3.6-review" - local list_output - list_output="$(ollama list 2>/dev/null || true)" - if printf '%s\n' "$list_output" | awk '{print $1}' | grep -Fxq "${custom}:latest"; then - echo "${custom} review model already exists." - return - fi - echo "Creating review model ${custom} (thinking disabled, 32K context)..." - local modelfile - modelfile="$(mktemp)" - cat > "$modelfile" < 0 ? defaultContextWindow : 131072 + this.defaultContextWindow = defaultContextWindow > 0 ? defaultContextWindow : Integer.parseInt(DEFAULT_CONTEXT_WINDOW) } int estimatedTokens(String sessionId) { diff --git a/src/main/groovy/se/alipsa/lca/intent/IntentRouterAgent.groovy b/src/main/groovy/se/alipsa/lca/intent/IntentRouterAgent.groovy index df893b4..a79814d 100644 --- a/src/main/groovy/se/alipsa/lca/intent/IntentRouterAgent.groovy +++ b/src/main/groovy/se/alipsa/lca/intent/IntentRouterAgent.groovy @@ -38,7 +38,7 @@ class IntentRouterAgent { ModelRegistry modelRegistry, IntentRouterParser parser, @Value('${assistant.intent.model:tinyllama}') String model, - @Value('${assistant.intent.fallback-model:gpt-oss:20b}') String fallbackModel, + @Value('${assistant.intent.fallback-model}') String fallbackModel, @Value('${assistant.intent.temperature:0.0}') double temperature, @Value('${assistant.intent.max-tokens:256}') int maxTokens, @Value('${assistant.intent.confidence-threshold:0.8}') double confidenceThreshold, diff --git a/src/main/groovy/se/alipsa/lca/memory/MemorySettings.groovy b/src/main/groovy/se/alipsa/lca/memory/MemorySettings.groovy index bcea4c1..84ff141 100644 --- a/src/main/groovy/se/alipsa/lca/memory/MemorySettings.groovy +++ b/src/main/groovy/se/alipsa/lca/memory/MemorySettings.groovy @@ -16,6 +16,8 @@ class MemorySettings { /** * Embedding model used to vectorise memory content and recall queries. + * Kept in sync with application.properties' lca.memory.embedding-model chain + * (MemorySettingsSpec asserts this default independently of Spring binding). */ String embeddingModel = "nomic-embed-text:latest" diff --git a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy index e020ec0..e255e0e 100644 --- a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy +++ b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy @@ -21,7 +21,24 @@ import java.util.regex.Pattern class CommandExecutor { private static final Logger log = LoggerFactory.getLogger(CommandExecutor) - private static final Pattern COMMAND_PATTERN = Pattern.compile(/^\/(\w+)\s*([\s\S]*)/) + // [\w-] (not \w alone) so hyphenated command names (/git-apply, /git-push) are captured whole + // instead of being truncated at the first hyphen. + private static final Pattern COMMAND_PATTERN = Pattern.compile(/^\/([\w-]+)\s*([\s\S]*)/) + + /** + * Command names this class actually dispatches in {@link #execute}'s switch (kept in sync with + * it by hand — small, stable list). Used by {@link #isKnownCommand} so the REPL can bypass the + * LLM intent classifier for input that's already an unambiguous, literal slash command: routing + * a verbatim "/benchmark --model x" through a small classifier risks it being reinterpreted as + * something else entirely (observed: misrouted to /run, which then tried to execute "benchmark" + * as a literal shell binary). + */ + private static final Set KNOWN_COMMANDS = Set.of( + "chat", "plan", "implement", "review", "search", "run", "edit", "paste", + "gitapply", "git-apply", "git-push", "apply", "status", "diff", "tree", "codesearch", + "mcp", "reviewlog", "compact", "help", "health", "benchmark", "exit", "quit", + "model", "context", "version", "stage", "revert", "commit-suggest", "applyblocks" + ) private final ShellCommands shellCommands private final McpCommands mcpCommands @@ -73,6 +90,8 @@ class CommandExecutor { case "gitapply": case "git-apply": return executeGitApply(args) + case "git-push": + return executeGitPush(args) case "apply": return executeApply(args) case "status": @@ -93,6 +112,22 @@ class CommandExecutor { return shellCommands.help() case "health": return shellCommands.health() + case "benchmark": + return executeBenchmark(args) + case "model": + return executeModel(args) + case "context": + return executeContext(args) + case "version": + return shellCommands.version() + case "stage": + return executeStage(args) + case "revert": + return executeRevert(args) + case "commit-suggest": + return executeCommitSuggest(args) + case "applyblocks": + return executeApplyBlocks(args) case "exit": case "quit": // Trigger system exit @@ -103,6 +138,19 @@ class CommandExecutor { } } + /** + * True when {@code input} is already a literal, well-formed slash command this class can + * dispatch on its own (e.g. "/benchmark --model x"). Callers use this to skip the LLM intent + * classifier entirely for unambiguous input, routing straight to {@link #execute}. + */ + boolean isKnownCommand(String input) { + if (input == null) { + return false + } + Matcher matcher = COMMAND_PATTERN.matcher(input.trim()) + matcher.matches() && KNOWN_COMMANDS.contains(matcher.group(1).toLowerCase()) + } + /** * Dispatch already-known paste content directly to ShellCommands.paste, * bypassing COMMAND_PATTERN/parseArgs entirely. Used by JLineRepl for @@ -182,7 +230,7 @@ class CommandExecutor { parseBoolean(parsed.staged) ?: false, parseSeverity(parsed.minSeverity, ReviewSeverity.LOW), parseBoolean(parsed.noColor) ?: false, - parseBoolean(parsed.logReview) ?: true, + parseBooleanFlag(parsed.logReview, true), parseBoolean(parsed.security) ?: false, parseBoolean(parsed.sast) ?: false, parseBoolean(parsed.withThinking) ?: parseBoolean(parsed.reasoning) ?: false, @@ -199,7 +247,7 @@ class CommandExecutor { parsed.session as String ?: "default", parsed.provider as String ?: "duckduckgo", parseLong(parsed.timeout) ?: 15000L, - parseBoolean(parsed.headless) ?: true, + parseBooleanFlag(parsed.headless, true), parsed.enableWebSearch != null ? parseBoolean(parsed.enableWebSearch) : null ) } @@ -212,7 +260,7 @@ class CommandExecutor { parseLong(parsed.timeout) ?: 60000L, parseInt(parsed.maxOutputChars) ?: 8000, parsed.session as String ?: "default", - parseBoolean(parsed.confirm) ?: true, + parseBooleanFlag(parsed.confirm, true), false // agentRequested ) } @@ -253,8 +301,8 @@ class CommandExecutor { patch, patchFile, parseBoolean(parsed.cached) ?: false, - parseBoolean(parsed.check) ?: true, - parseBoolean(parsed.confirm) ?: true + parseBooleanFlag(parsed.check, true), + parseBooleanFlag(parsed.confirm, true) ) } @@ -265,8 +313,8 @@ class CommandExecutor { shellCommands.applyPatch( patch, patchFile, - parseBoolean(parsed.dryRun) ?: true, - parseBoolean(parsed.confirm) ?: true + parseBooleanFlag(parsed.dryRun, true), + parseBooleanFlag(parsed.confirm, true) ) } @@ -330,6 +378,104 @@ class CommandExecutor { ) } + private String executeGitPush(String args) { + Map parsed = parseArgs(args) + shellCommands.gitPush( + parseBoolean(parsed.force) ?: false, + parseBooleanFlag(parsed.confirm, true) + ) + } + + private String executeBenchmark(String args) { + Map parsed = parseArgs(args) + // Not "parseInt(...) ?: 200": Groovy truthiness treats 0 as falsy, so an explicit + // "--max-tokens 0" would otherwise silently become 200 instead of being rejected. + Integer maxTokens = parsed.maxTokens != null ? parseInt(parsed.maxTokens) : null + shellCommands.benchmark( + parsed.model as String, + parsed.prompt as String, + parsed.promptFile as String, + maxTokens != null ? maxTokens : 200, + parsed.session as String ?: "default" + ) + } + + private String executeModel(String args) { + Map parsed = parseArgs(args) + shellCommands.model( + parsed.set as String, + parsed.session as String ?: "default", + parseBoolean(parsed.list) ?: false + ) + } + + private String executeContext(String args) { + Map parsed = parseArgs(args) + String filePath = parsed.filePath as String ?: firstWord(parsed) + // Not "parseInt(...) ?: 2": ShellCommands.context accepts --padding 0 (requireMin(padding, + // 0, ...)), but Groovy's ?: treats a parsed 0 as absent, same trap fixed for /benchmark's + // --max-tokens 0. + Integer padding = parsed.padding != null ? parseInt(parsed.padding) : null + shellCommands.context( + filePath, + parseInt(parsed.start), + parseInt(parsed.end), + parsed.symbol as String, + padding != null ? padding : 2 + ) + } + + private String executeStage(String args) { + Map parsed = parseArgs(args) + List paths = null + if (parsed.paths) { + paths = (parsed.paths as String).split(',').toList() + } else if (parsed.words && !(parsed.words as List).isEmpty()) { + paths = parsed.words as List + } + shellCommands.stage( + paths, + parsed.file as String, + parsed.hunks as String, + parseBooleanFlag(parsed.confirm, true) + ) + } + + private String executeRevert(String args) { + Map parsed = parseArgs(args) + String filePath = parsed.filePath as String ?: firstWord(parsed) + shellCommands.revert( + filePath, + parseBooleanFlag(parsed.dryRun, false), + parseBooleanFlag(parsed.confirm, true) + ) + } + + private String executeCommitSuggest(String args) { + Map parsed = parseArgs(args) + shellCommands.commitSuggest( + parsed.session as String ?: "default", + parsed.model as String, + parsed.temperature as Double, + parsed.maxTokens as Integer, + parsed.hint as String, + parseBooleanFlag(parsed.secretScan, true), + parseBooleanFlag(parsed.allowSecrets, false) + ) + } + + private String executeApplyBlocks(String args) { + Map parsed = parseArgs(args) + String filePath = parsed.filePath as String ?: firstWord(parsed) + shellCommands.applyBlocks( + filePath, + parsed.blocks as String, + parsed.blocksFile as String, + parseBooleanFlag(parsed.dryRun, true), + parseBooleanFlag(parsed.confirm, true) + ) + } + private String executeCompact(String args) { Map parsed = parseArgs(args) shellCommands.compact(parsed.session as String ?: "default") @@ -427,6 +573,15 @@ class CommandExecutor { return words ? words.join(" ") : "" } + /** + * First positional word, for commands whose required file-path argument is more natural typed + * bare (e.g. "/context src/Foo.groovy --symbol bar") than behind an explicit --file-path flag. + */ + private String firstWord(Map parsed) { + List words = parsed.words as List + words && !words.isEmpty() ? words[0] : null + } + /** Normalizes a kebab-case CLI flag name (e.g. {@code no-color}) to the camelCase map key * every {@code executeXxx} method reads (e.g. {@code noColor}). A no-op for flags with no * hyphen, so already-camelCase flags like {@code --maxTokens} are unaffected. */ @@ -456,6 +611,18 @@ class CommandExecutor { } } + /** + * Resolves a boolean flag against a non-false default without Groovy's {@code ?:} truthiness + * trap: {@code parseBoolean(value) ?: defaultValue} silently turns an explicit "--flag false" + * back into {@code defaultValue} whenever that default is {@code true}, since Elvis treats the + * parsed {@code false} itself as absent (the same class of bug fixed for /benchmark's + * --max-tokens 0). Only missing/unparsable input falls back to {@code defaultValue}. + */ + private boolean parseBooleanFlag(Object value, boolean defaultValue) { + Boolean parsed = parseBoolean(value) + parsed != null ? parsed : defaultValue + } + private Boolean parseBoolean(Object value) { if (value == null) return null if (value instanceof Boolean) return (Boolean) value diff --git a/src/main/groovy/se/alipsa/lca/repl/JLineRepl.groovy b/src/main/groovy/se/alipsa/lca/repl/JLineRepl.groovy index c16aed5..a2763fb 100644 --- a/src/main/groovy/se/alipsa/lca/repl/JLineRepl.groovy +++ b/src/main/groovy/se/alipsa/lca/repl/JLineRepl.groovy @@ -166,6 +166,14 @@ class JLineRepl { return } + if (commandExecutor.isKnownCommand(trimmed)) { + String result = commandExecutor.execute(trimmed) + if (result != null && !result.trim().isEmpty()) { + terminal.writer().println(result) + } + return + } + processInput(trimmed) } diff --git a/src/main/groovy/se/alipsa/lca/shell/SessionState.groovy b/src/main/groovy/se/alipsa/lca/shell/SessionState.groovy index beb0247..7aa8d38 100644 --- a/src/main/groovy/se/alipsa/lca/shell/SessionState.groovy +++ b/src/main/groovy/se/alipsa/lca/shell/SessionState.groovy @@ -46,7 +46,7 @@ class SessionState { private final LocalOnlyState localOnlyState SessionState( - @Value('${assistant.llm.model:qwen3.6:35b-a3b}') String defaultModel, + @Value('${assistant.llm.model}') String defaultModel, @Value('${assistant.llm.temperature.craft:0.7}') double defaultCraftTemperature, @Value('${assistant.llm.temperature.review:0.35}') double defaultReviewTemperature, @Value('${assistant.llm.max-tokens:0}') Integer defaultMaxTokens, diff --git a/src/main/groovy/se/alipsa/lca/shell/ShellCommands.groovy b/src/main/groovy/se/alipsa/lca/shell/ShellCommands.groovy index d9dae75..7953d59 100644 --- a/src/main/groovy/se/alipsa/lca/shell/ShellCommands.groovy +++ b/src/main/groovy/se/alipsa/lca/shell/ShellCommands.groovy @@ -228,6 +228,7 @@ Do not execute any commands. commands.put("/!", "Execute a shell command directly (alias: /sh).") commands.put("/apply", "Apply a unified diff patch with confirmation.") commands.put("/applyBlocks", "Apply Search-and-Replace blocks to a file.") + commands.put("/benchmark", "Measure raw Ollama inference speed (tokens/sec) for a model.") commands.put("/chat", "Send a prompt to the coding assistant.") commands.put("/codesearch", "Search repository files with ripgrep.") commands.put("/commit-suggest", "Draft a commit message from staged changes.") @@ -1327,6 +1328,73 @@ Try: "Ollama unreachable at ${modelRegistry.getBaseUrl()}: ${health.message}" } + private static final String DEFAULT_BENCHMARK_PROMPT = + "Write a one-paragraph explanation of how binary search works, then show a short example in Groovy." + + @ShellMethod( + key = ["/benchmark"], + value = "Measure raw Ollama inference speed (tokens/sec) for a model." + ) + String benchmark( + @ShellOption(defaultValue = ShellOption.NULL, help = "Model to benchmark (default: active session model)") String model, + @ShellOption(defaultValue = ShellOption.NULL, help = "Prompt text to send") String prompt, + @ShellOption(defaultValue = ShellOption.NULL, help = "Read the prompt from a file instead of --prompt") String promptFile, + @ShellOption(defaultValue = "200", help = "Max tokens to generate") int maxTokens, + @ShellOption(defaultValue = "default", help = "Session id") String session + ) { + if (maxTokens <= 0) { + return "Max tokens must be positive." + } + ModelRegistry.Health health = modelRegistry.checkHealth() + if (!health.reachable) { + return "Ollama unreachable at ${modelRegistry.getBaseUrl()}: ${health.message}" + } + String targetModel = model ?: (sessionState.getOrCreate(session).getModel() ?: sessionState.getDefaultModel()) + if (!targetModel) { + return "No model configured. Specify one with /benchmark --model ." + } + List available = modelRegistry.listModels() + if (!available.isEmpty() && !available.any { it.equalsIgnoreCase(targetModel) }) { + return "Model '${targetModel}' not found. Available: ${String.join(', ', available)}" + } + String effectivePrompt + if (promptFile) { + try { + effectivePrompt = fileEditingTool.readFile(promptFile) + } catch (IllegalArgumentException e) { + return "Could not read prompt file: ${e.message}" + } + } else { + effectivePrompt = prompt ?: DEFAULT_BENCHMARK_PROMPT + } + if (effectivePrompt.trim().isEmpty()) { + return "Prompt is empty." + } + ModelRegistry.BenchmarkResult result + try { + result = modelRegistry.benchmark(targetModel, effectivePrompt, maxTokens) + } catch (Exception e) { + return "Benchmark failed: ${e.message}" + } + Integer contextLength = modelRegistry.contextLength(targetModel) + StringBuilder body = new StringBuilder() + body.append("Model: ").append(targetModel).append("\n") + if (contextLength != null) { + body.append("Reported context length: ").append(contextLength).append(" tokens\n") + } + body.append("Prompt tokens: ").append(result.promptEvalCount) + .append(" (").append(String.format(Locale.ROOT, "%.2f", result.promptTokensPerSecond)).append(" tok/s)\n") + body.append("Generated tokens: ").append(result.evalCount) + .append(" (").append(String.format(Locale.ROOT, "%.2f", result.evalTokensPerSecond)).append(" tok/s)\n") + body.append("Load duration: ").append(formatDurationSeconds(result.loadDurationNanos)).append("\n") + body.append("Total duration: ").append(formatDurationSeconds(result.totalDurationNanos)) + formatSection("Benchmark", body.toString()) + } + + private static String formatDurationSeconds(long nanos) { + String.format(Locale.ROOT, "%.2fs", nanos / 1_000_000_000d) + } + @ShellMethod( key = ["/!", "/sh"], value = "Execute a shell command directly with streaming output." diff --git a/src/main/groovy/se/alipsa/lca/team/TeamSettings.groovy b/src/main/groovy/se/alipsa/lca/team/TeamSettings.groovy index cc138eb..133e82a 100644 --- a/src/main/groovy/se/alipsa/lca/team/TeamSettings.groovy +++ b/src/main/groovy/se/alipsa/lca/team/TeamSettings.groovy @@ -25,10 +25,10 @@ class TeamSettings { TeamSettings( @Value('${assistant.team.enabled:false}') boolean enabled, - @Value('${assistant.team.architect-model:${assistant.llm.model:qwen3.6-128k:latest}}') String architectModel, - @Value('${assistant.team.engineer-model:${assistant.llm.model:qwen3.6-128k:latest}}') String engineerModel, - @Value('${assistant.team.dispatcher-model:${assistant.llm.model:qwen3.6-128k:latest}}') String dispatcherModel, - @Value('${assistant.team.reviewer-model:${assistant.llm.model:qwen3.6-128k:latest}}') String reviewerModel, + @Value('${assistant.team.architect-model:${assistant.llm.model}}') String architectModel, + @Value('${assistant.team.engineer-model:${assistant.llm.model}}') String engineerModel, + @Value('${assistant.team.dispatcher-model:${assistant.llm.model}}') String dispatcherModel, + @Value('${assistant.team.reviewer-model:${assistant.llm.model}}') String reviewerModel, @Value('${assistant.team.dispatcher-temperature:0.1}') double dispatcherTemperature, @Value('${assistant.team.architect-temperature:0.3}') double architectTemperature, @Value('${assistant.team.engineer-temperature:0.2}') double engineerTemperature, diff --git a/src/main/groovy/se/alipsa/lca/tools/ModelRegistry.groovy b/src/main/groovy/se/alipsa/lca/tools/ModelRegistry.groovy index 973705a..9a5148c 100644 --- a/src/main/groovy/se/alipsa/lca/tools/ModelRegistry.groovy +++ b/src/main/groovy/se/alipsa/lca/tools/ModelRegistry.groovy @@ -32,9 +32,11 @@ class ModelRegistry { private final URI tagsUri private final URI showUri private final URI psUri + private final URI generateUri private final boolean remote private final HttpClient client private final Duration timeout + private final Duration benchmarkTimeout private final String baseUrl private final long cacheTtlMillis private final long healthTtlMillis @@ -50,6 +52,7 @@ class ModelRegistry { @Value('${assistant.llm.registry-timeout-millis:4000}') long timeoutMillis, @Value('${assistant.llm.model-cache-ttl-millis:30000}') long cacheTtlMillis, @Value('${assistant.llm.health-cache-ttl-millis:5000}') long healthTtlMillis, + @Value('${assistant.llm.benchmark-timeout-millis:180000}') long benchmarkTimeoutMillis, @Nullable HttpClient httpClient ) { if (baseUrl == null || baseUrl.trim().isEmpty()) { @@ -60,9 +63,12 @@ class ModelRegistry { this.tagsUri = URI.create("${normalized}/api/tags") this.showUri = URI.create("${normalized}/api/show") this.psUri = URI.create("${normalized}/api/ps") + this.generateUri = URI.create("${normalized}/api/generate") this.remote = isRemoteHost(tagsUri.getHost()) long effectiveTimeout = timeoutMillis > 0 ? timeoutMillis : 4000L this.timeout = Duration.ofMillis(effectiveTimeout) + long effectiveBenchmarkTimeout = benchmarkTimeoutMillis > 0 ? benchmarkTimeoutMillis : 180000L + this.benchmarkTimeout = Duration.ofMillis(effectiveBenchmarkTimeout) this.cacheTtlMillis = cacheTtlMillis > 0 ? cacheTtlMillis : 30000L this.healthTtlMillis = healthTtlMillis > 0 ? healthTtlMillis : 5000L this.client = httpClient != null @@ -296,6 +302,41 @@ class ModelRegistry { null } + /** + * Calls Ollama's {@code /api/generate} directly (bypassing Spring AI/Embabel, which doesn't + * surface Ollama's raw timing fields) to measure raw inference speed for {@code model}. Unlike + * {@link #listModels}/{@link #checkHealth}/{@link #contextLength} (which degrade gracefully on + * failure since they're polled implicitly for UI purposes), this throws on failure: it's a + * deliberate, one-off diagnostic action, so surfacing the real Ollama error is more useful than + * a silent null. + */ + BenchmarkResult benchmark(String model, String prompt, int maxTokens) throws IOException { + HttpResponse response = fetchGenerate(model, prompt, maxTokens) + if (response.statusCode() < 200 || response.statusCode() >= 300) { + throw new IOException("Ollama returned status ${response.statusCode()} for model '${model}': ${response.body()}".toString()) + } + Map parsed = (Map) new JsonSlurper().parseText(response.body()) + new BenchmarkResult( + model, + asLong(parsed.get("total_duration")), + asLong(parsed.get("load_duration")), + (int) asLong(parsed.get("prompt_eval_count")), + asLong(parsed.get("prompt_eval_duration")), + (int) asLong(parsed.get("eval_count")), + asLong(parsed.get("eval_duration")) + ) + } + + protected HttpResponse fetchGenerate(String model, String prompt, int maxTokens) throws Exception { + String body = JsonOutput.toJson([model: model, prompt: prompt, stream: false, options: [num_predict: maxTokens]]) + HttpRequest request = HttpRequest.newBuilder(generateUri) + .timeout(benchmarkTimeout) + .header("Content-Type", "application/json") + .POST(HttpRequest.BodyPublishers.ofString(body)) + .build() + client.send(request, HttpResponse.BodyHandlers.ofString()) + } + protected HttpResponse fetchShow(String model) throws Exception { String body = JsonOutput.toJson([name: model]) HttpRequest request = HttpRequest.newBuilder(showUri) @@ -349,6 +390,26 @@ class ModelRegistry { long sizeVram // portion resident in GPU VRAM, bytes (0 if CPU-only) } + @Canonical + @CompileStatic + static class BenchmarkResult { + String model + long totalDurationNanos + long loadDurationNanos + int promptEvalCount + long promptEvalDurationNanos + int evalCount + long evalDurationNanos + + double getPromptTokensPerSecond() { + promptEvalDurationNanos > 0 ? promptEvalCount / (promptEvalDurationNanos / 1_000_000_000d) : 0d + } + + double getEvalTokensPerSecond() { + evalDurationNanos > 0 ? evalCount / (evalDurationNanos / 1_000_000_000d) : 0d + } + } + String getBaseUrl() { baseUrl } diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index de17e92..ad30376 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -11,27 +11,34 @@ logging.level.se.alipsa.lca.repl.ReplRunner=info # Changing default LLM -embabel.models.default-llm=qwen3.6-128k:latest -embabel.models.default-embedding-model=nomic-embed-text:latest +# These two properties are the canonical model definitions for the whole app. They accept +# an override from the LCA_* environment variables that src/main/bin/lca exports at launch +# (the values there are, in turn, the single canonical source shared with models.sh); the +# literal after the ":" is only a fallback for runs that bypass lca (./run.sh, mvnw test, IDE). +embabel.models.default-llm=${LCA_CHAT_MODEL:qwen3.8-192k:latest} +embabel.models.default-embedding-model=${LCA_EMBEDDING_MODEL:nomic-embed-text:latest} # Llm Roles: Create as many as you want and use with byRole("role-name") -embabel.models.llms.best=qwen3.6-128k:latest -embabel.models.llms.cheapest=gpt-oss-64k:latest +embabel.models.llms.best=${embabel.models.default-llm} +embabel.models.llms.cheapest=${LCA_FALLBACK_MODEL:gpt-oss-64k:latest} -embabel.models.embedding-services.best=nomic-embed-text:latest -embabel.models.embedding-services.cheapest=nomic-embed-text:latest +embabel.models.embedding-services.best=${embabel.models.default-embedding-model} +embabel.models.embedding-services.cheapest=${embabel.models.default-embedding-model} # Set this to use your preferred LLM for ranking, to avoid default -embabel.agent-platform.ranking.llm=gpt-oss-64k:latest +embabel.agent-platform.ranking.llm=${embabel.models.llms.cheapest} # Coding assistant LLM defaults -assistant.llm.model=${embabel.models.default-llm:qwen3.6-128k:latest} -assistant.llm.fallback-model=${embabel.models.llms.cheapest:gpt-oss-64k:latest} -assistant.llm.review-model=qwen3.6-review:latest +assistant.llm.model=${embabel.models.default-llm} +assistant.llm.fallback-model=${embabel.models.llms.cheapest} +assistant.llm.review-model=${LCA_REVIEW_MODEL:qwen3.8-review:latest} assistant.llm.temperature.craft=0.7 assistant.llm.temperature.review=0.1 -# LLM call timeout in milliseconds (default 10 minutes; increase for large reviews on local models) -assistant.llm.timeout-millis=600000 +# LLM call timeout in milliseconds (default 12 minutes; increase for large reviews on local models) +assistant.llm.timeout-millis=720000 +# Timeout for /benchmark's raw Ollama /api/generate call (separate from the short registry +# timeout used for tags/show/ps polling, since a generation call can legitimately take minutes). +assistant.llm.benchmark-timeout-millis=180000 # Local-only mode also disables web search in SessionState, so no separate web-search flag is needed assistant.local-only=true # Shell input behaviour @@ -43,10 +50,14 @@ assistant.thinking.enabled=true # Whether to enable thinking by default for review commands (can be overridden with --with-thinking flag) assistant.thinking.review-default=false -# Maximum characters for general review context (directory file contents) -assistant.llm.review-context-budget=30000 +# Maximum characters for general review context (directory file contents). The review +# model's context window is 131072 tokens; these budgets are deliberately well under a +# naive chars-per-token conversion of that, leaving headroom for the prompt template, +# system prompt, and the model's own output. Raise cautiously and watch for context +# overflow - code tends to tokenize less efficiently than prose. +assistant.llm.review-context-budget=100000 # Maximum characters for PR review context (diff + file contents) -assistant.llm.review-pr-context-budget=80000 +assistant.llm.review-pr-context-budget=250000 # Web search fetcher (htmlunit or jsoup). Set fallback-fetcher=none to disable fallback. assistant.web-search.fetcher=htmlunit @@ -56,11 +67,11 @@ assistant.tool-summary.ttl-seconds=600 # Intent routing assistant.intent.enabled=true -assistant.intent.model=gpt-oss-64k:latest -assistant.intent.fallback-model=qwen3.6-128k:latest +assistant.intent.model=${embabel.models.llms.cheapest} +assistant.intent.fallback-model=${embabel.models.default-llm} assistant.intent.temperature=0.1 assistant.intent.max-tokens=256 -assistant.intent.allowed-commands=/chat,/plan,/review,/implement,/edit,/apply,/run,/gitapply,/git-push,/search,/codesearch,/diff,/stage,/commit-suggest,/context,/tree,/status,/model,/health,/revert,/applyBlocks,/paste,/version,/mcp,/compact,/reviewlog +assistant.intent.allowed-commands=/chat,/plan,/review,/implement,/edit,/apply,/run,/gitapply,/git-push,/search,/codesearch,/diff,/stage,/commit-suggest,/context,/tree,/status,/model,/health,/benchmark,/revert,/applyBlocks,/paste,/version,/mcp,/compact,/reviewlog assistant.intent.destructive-commands=/edit,/apply,/run,/gitapply,/git-push assistant.intent.confidence-threshold=0.8 assistant.intent.second-opinion-threshold=0.6 @@ -93,11 +104,11 @@ lca.repl.history-file=${user.home}/.lca/history # Swing GUI (lcaGui). Disabled by default; the lcaGui launcher enables it and disables the REPL. lca.gui.enabled=false # Fallback context-window size (tokens) used for the footer context gauge when Ollama does not report one. -lca.gui.default-context-window=131072 +lca.gui.default-context-window=${LCA_DEFAULT_CONTEXT_WINDOW:131072} # Long-term memory (RAG-based recall/remember) - roadmap item 3, step 1 lca.memory.enabled=true -lca.memory.embedding-model=${embabel.models.embedding-services.cheapest:nomic-embed-text:latest} +lca.memory.embedding-model=${embabel.models.embedding-services.cheapest} lca.memory.index-directory=${user.home}/.lca/memory-index lca.memory.recall-top-k=5 lca.memory.recall-min-score=0.6 diff --git a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy index ebb9be7..923ebea 100644 --- a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy @@ -5,6 +5,7 @@ import se.alipsa.lca.review.ReviewSeverity import se.alipsa.lca.shell.McpCommands import se.alipsa.lca.shell.ShellCommands import spock.lang.Specification +import spock.lang.Unroll class CommandExecutorSpec extends Specification { @@ -34,6 +35,15 @@ class CommandExecutorSpec extends Specification { result == "Invalid command format. Expected: /command [args]" } + def "execute respects an explicit /review --log-review false instead of forcing logging back on"() { + when: + executor.execute('/review --code "x" --log-review false') + + then: + 1 * shellCommands.review("x", "", "default", null, null, null, null, null, false, + ReviewSeverity.LOW, false, false, false, false, false, null) >> "reviewed" + } + def "execute forwards the --command flag text to ShellCommands.runCommand"() { given: String command = '/run --command "gh pr list --state open"' @@ -46,6 +56,290 @@ class CommandExecutorSpec extends Specification { result == "ran" } + def "execute respects an explicit /run --confirm false instead of forcing confirmation back on"() { + when: + executor.execute('/run --command "ls" --confirm false') + + then: + 1 * shellCommands.runCommand("ls", 60000L, 8000, "default", false, false) >> "ran" + } + + def "execute defaults /search's headless flag to true"() { + when: + executor.execute('/search foo') + + then: + 1 * shellCommands.search("foo", 5, "default", "duckduckgo", 15000L, true, null) >> "results" + } + + def "execute respects an explicit /search --headless false instead of forcing it back on"() { + when: + executor.execute('/search foo --headless false') + + then: + 1 * shellCommands.search("foo", 5, "default", "duckduckgo", 15000L, false, null) >> "results" + } + + def "execute parses /benchmark flags and forwards them to ShellCommands.benchmark"() { + given: + String command = '/benchmark --model qwen3.8-review --prompt-file review-prompt.txt --max-tokens 50' + + when: + String result = executor.execute(command) + + then: + 1 * shellCommands.benchmark("qwen3.8-review", null, "review-prompt.txt", 50, "default") >> "benchmarked" + result == "benchmarked" + } + + def "execute defaults /benchmark's max-tokens and session when not given"() { + when: + executor.execute('/benchmark') + + then: + 1 * shellCommands.benchmark(null, null, null, 200, "default") >> "benchmarked" + } + + def "execute parses /git-push flags and forwards them to ShellCommands.gitPush"() { + when: + String result = executor.execute('/git-push --force') + + then: + 1 * shellCommands.gitPush(true, true) >> "pushed" + result == "pushed" + } + + def "execute defaults /git-push's force and confirm when not given"() { + when: + executor.execute('/git-push') + + then: + 1 * shellCommands.gitPush(false, true) >> "pushed" + } + + def "execute respects an explicit /git-push --confirm false instead of forcing confirmation back on"() { + // Regression guard: "parseBoolean(x) ?: true" treats a parsed `false` as absent under + // Groovy's Elvis operator, silently re-enabling the confirmation prompt even when the + // caller explicitly asked to skip it. + when: + executor.execute('/git-push --confirm false') + + then: + 1 * shellCommands.gitPush(false, false) >> "pushed" + } + + def "execute parses /gitapply flags and forwards them to ShellCommands.gitApply"() { + when: + String result = executor.execute('/gitapply --patch-file p.diff --cached') + + then: + 1 * shellCommands.gitApply(null, "p.diff", true, true, true) >> "applied" + result == "applied" + } + + def "execute respects an explicit /gitapply --check false and --confirm false"() { + when: + executor.execute('/gitapply --patch-file p.diff --check false --confirm false') + + then: + 1 * shellCommands.gitApply(null, "p.diff", false, false, false) >> "applied" + } + + def "execute parses /apply flags and forwards them to ShellCommands.applyPatch"() { + when: + String result = executor.execute('/apply --patch-file p.diff') + + then: + 1 * shellCommands.applyPatch("", "p.diff", true, true) >> "applied" + result == "applied" + } + + def "execute respects an explicit /apply --dry-run false and --confirm false"() { + when: + executor.execute('/apply --patch-file p.diff --dry-run false --confirm false') + + then: + 1 * shellCommands.applyPatch("", "p.diff", false, false) >> "applied" + } + + def "execute parses /model flags and forwards them to ShellCommands.model"() { + when: + String result = executor.execute('/model --set gpt-oss:20b --session s1') + + then: + 1 * shellCommands.model("gpt-oss:20b", "s1", false) >> "model set" + result == "model set" + } + + def "execute defaults /model's session and list when not given"() { + when: + executor.execute('/model --list') + + then: + 1 * shellCommands.model(null, "default", true) >> "models" + } + + def "execute parses /context flags and forwards them to ShellCommands.context"() { + when: + String result = executor.execute('/context --file-path src/Foo.groovy --start 10 --end 20 --padding 5') + + then: + 1 * shellCommands.context("src/Foo.groovy", 10, 20, null, 5) >> "context" + result == "context" + } + + def "execute accepts /context's file path as a positional word"() { + when: + executor.execute('/context src/Foo.groovy --symbol myMethod') + + then: + 1 * shellCommands.context("src/Foo.groovy", null, null, "myMethod", 2) >> "context" + } + + def "execute respects an explicit /context --padding 0 instead of falling back to the default"() { + // Regression guard: ShellCommands.context accepts 0 padding (requireMin(padding, 0, ...)), + // but "parseInt(x) ?: 2" would treat a parsed 0 as absent - the same trap fixed for + // /benchmark's --max-tokens 0. + when: + executor.execute('/context src/Foo.groovy --symbol myMethod --padding 0') + + then: + 1 * shellCommands.context("src/Foo.groovy", null, null, "myMethod", 0) >> "context" + } + + def "execute dispatches /version to ShellCommands.version"() { + when: + String result = executor.execute('/version') + + then: + 1 * shellCommands.version() >> "lca version: 1.0" + result == "lca version: 1.0" + } + + def "execute parses /stage flags and forwards them to ShellCommands.stage"() { + when: + String result = executor.execute('/stage --file build.gradle --hunks 1,2 --confirm false') + + then: + 1 * shellCommands.stage(null, "build.gradle", "1,2", false) >> "staged" + result == "staged" + } + + def "execute passes /stage's positional words as paths"() { + when: + executor.execute('/stage a.txt b.txt') + + then: + 1 * shellCommands.stage(["a.txt", "b.txt"], null, null, true) >> "staged" + } + + def "execute parses /revert flags and forwards them to ShellCommands.revert"() { + when: + String result = executor.execute('/revert --file-path src/Foo.groovy --dry-run true') + + then: + 1 * shellCommands.revert("src/Foo.groovy", true, true) >> "reverted" + result == "reverted" + } + + def "execute accepts /revert's file path as a positional word"() { + when: + executor.execute('/revert src/Foo.groovy') + + then: + 1 * shellCommands.revert("src/Foo.groovy", false, true) >> "reverted" + } + + def "execute parses /commit-suggest flags and forwards them to ShellCommands.commitSuggest"() { + when: + String result = executor.execute('/commit-suggest --session s1 --hint "fix bug" --allow-secrets true') + + then: + 1 * shellCommands.commitSuggest("s1", null, null, null, "fix bug", true, true) >> "suggested" + result == "suggested" + } + + def "execute defaults /commit-suggest's session and flags when not given"() { + when: + executor.execute('/commit-suggest') + + then: + 1 * shellCommands.commitSuggest("default", null, null, null, null, true, false) >> "suggested" + } + + def "execute respects an explicit /commit-suggest --secret-scan false instead of falling back to true"() { + // Regression guard: Groovy's ?: treats a parsed `false` as absent, so a naive + // "parseBoolean(x) ?: true" would silently re-enable scanning here (the same + // truthiness trap fixed for /benchmark's --max-tokens 0). + when: + executor.execute('/commit-suggest --secret-scan false') + + then: + 1 * shellCommands.commitSuggest("default", null, null, null, null, false, false) >> "suggested" + } + + def "execute parses /applyBlocks flags and forwards them to ShellCommands.applyBlocks"() { + when: + String result = executor.execute('/applyBlocks --file-path src/Foo.groovy --blocks-file blocks.txt --dry-run false') + + then: + 1 * shellCommands.applyBlocks("src/Foo.groovy", null, "blocks.txt", false, true) >> "applied" + result == "applied" + } + + def "execute accepts /applyBlocks's file path as a positional word"() { + when: + executor.execute('/applyBlocks src/Foo.groovy --blocks "some blocks"') + + then: + 1 * shellCommands.applyBlocks("src/Foo.groovy", "some blocks", null, true, true) >> "applied" + } + + @Unroll + def "'/#name' dispatches instead of falling through to the unknown-command fallback"() { + // Guards against the asymmetric drift risk this class's KNOWN_COMMANDS set otherwise has: + // a case added to execute()'s switch but not mirrored into KNOWN_COMMANDS would keep + // routing that command through the LLM intent classifier instead of dispatching it + // directly - exactly the bug /benchmark hit. This doesn't prove the converse (a stale + // KNOWN_COMMANDS entry whose switch case was removed), only that everything the bypass + // claims to handle actually does. + when: + String result = executor.execute("/${name}") + + then: + result != "Unknown command: /${name}. Type /help for available commands." + executor.isKnownCommand("/${name}") + + where: + name << [ + "chat", "plan", "implement", "review", "search", "run", "edit", "paste", + "gitapply", "git-apply", "git-push", "apply", "status", "diff", "tree", "codesearch", + "mcp", "reviewlog", "compact", "help", "health", "benchmark", + "model", "context", "version", "stage", "revert", "commit-suggest", "applyblocks" + ] + } + + @Unroll + def "isKnownCommand('#input') == #expected"() { + expect: + executor.isKnownCommand(input) == expected + + where: + input || expected + "/health" || true + "/benchmark --model x" || true + "/BENCHMARK --model x" || true + "/review --code \"x\"" || true + "/gitapply --patch x" || true + "/git-apply --patch x" || true + "/git-push --force" || true + "/model --set foo" || true + "/frobnicate" || false + "review this please" || false + null || false + "" || false + } + def "executePasteContent forwards directly to ShellCommands.paste without re-parsing"() { given: String content = "/review --code \"whatever\"\nmore lines that would break COMMAND_PATTERN reparsing" diff --git a/src/test/groovy/se/alipsa/lca/repl/JLineReplSpec.groovy b/src/test/groovy/se/alipsa/lca/repl/JLineReplSpec.groovy index 7cc2889..eb6a98a 100644 --- a/src/test/groovy/se/alipsa/lca/repl/JLineReplSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/repl/JLineReplSpec.groovy @@ -85,4 +85,26 @@ class JLineReplSpec extends Specification { 1 * bangCommandHandler.handle("! git status", "default", false) >> "Command: git status\nExit: 0 (success)" 0 * intentRouter.routeDetails(_) } + + def "a literal known slash command dispatches directly, bypassing intent routing"() { + when: + repl.handleInput("/benchmark --model qwen3.8-review --prompt-file x.groovy") + + then: + 1 * commandExecutor.isKnownCommand("/benchmark --model qwen3.8-review --prompt-file x.groovy") >> true + 1 * commandExecutor.execute("/benchmark --model qwen3.8-review --prompt-file x.groovy") >> "Model: qwen3.8-review" + 0 * intentRouter.routeDetails(_) + } + + def "an unrecognized slash command still routes through the intent classifier"() { + given: + def plan = new IntentRoutingPlan(commands: [], confidence: 1.0d, explanation: null) + + when: + repl.handleInput("/frobnicate") + + then: + 1 * commandExecutor.isKnownCommand("/frobnicate") >> false + 1 * intentRouter.routeDetails("/frobnicate") >> new IntentRoutingOutcome(plan: plan, result: null) + } } diff --git a/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy b/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy new file mode 100644 index 0000000..52b1afd --- /dev/null +++ b/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy @@ -0,0 +1,133 @@ +package se.alipsa.lca.scripts + +import spock.lang.Specification +import spock.lang.Unroll + +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.Paths +import java.util.concurrent.TimeUnit +import java.util.stream.Collectors + +/** + * Runs every demo/test_*.sh script as part of ./mvnw test, so a demo test script actually + * gets executed instead of only being runnable by hand. A hand-authored bash test for + * _quant_suffix existed and would have caught the set -e/exit-1 regression, but nothing wired + * it into CI or ./mvnw test, so the bug shipped anyway. + */ +class DemoTestScriptsSpec extends Specification { + + private static final long TIMEOUT_SECONDS = 120L + private static final long GIT_TIMEOUT_SECONDS = 30L + + @Unroll + def "demo test script passes: #scriptName"() { + given: + Path scriptPath = demoDir().resolve(scriptName) + ProcessBuilder processBuilder = new ProcessBuilder("bash", scriptPath.toString()) + processBuilder.directory(demoDir().toFile()) + processBuilder.redirectErrorStream(true) + + when: + ProcessResult result = runBounded(processBuilder, TIMEOUT_SECONDS) + + then: + result.finished + result.exitCode == 0 + !result.output.contains("FAIL:") + + where: + scriptName << demoTestScriptNames() + } + + private static Path demoDir() { + Paths.get("").toAbsolutePath().normalize().resolve("demo") + } + + private static List demoTestScriptNames() { + List tracked = gitTrackedDemoTestScripts() + List names = Files.list(demoDir()).withCloseable { stream -> + stream.map { it.fileName.toString() } + .filter { it.startsWith("test_") && it.endsWith(".sh") && it != "test_helpers.sh" } + .filter { tracked.contains(it) } + .sorted() + .collect(Collectors.toList()) + } + // Spock already errors ("Data provider has no data") on an empty where: list rather than + // silently running zero iterations, but that generic message doesn't say WHY the list is + // empty. Fail with a diagnosis instead: this filters to git-tracked names, so an empty + // result almost certainly means the tracked-files lookup came back empty (e.g. demo/ not + // tracked from this checkout), not that the demo/ directory itself has no test scripts. + assert !names.isEmpty() : "no demo test scripts discovered under ${demoDir()} " + + "(${tracked.size()} git-tracked demo/ entries) - check gitTrackedDemoTestScripts()" + names + } + + /** + * Restricts discovery to files git actually tracks under demo/, so an untracked scratch + * script (e.g. a developer's local demo/test_whatever.sh) doesn't silently join the build. + * Falls back to "no filtering" (unfiltered disk listing) only if the git subprocess itself + * fails or times out, since a missing/hung git is an environment problem this spec shouldn't + * mask a script list over. + */ + private static List gitTrackedDemoTestScripts() { + ProcessBuilder processBuilder = new ProcessBuilder("git", "ls-files", "demo") + processBuilder.directory(demoDir().parent.toFile()) + processBuilder.redirectErrorStream(true) + ProcessResult result = runBounded(processBuilder, GIT_TIMEOUT_SECONDS) + if (!result.finished || result.exitCode != 0) { + return demoTestScriptNamesOnDisk() + } + result.output.readLines() + .findAll { it.startsWith("demo/") } + .collect { it.substring("demo/".length()) } + } + + private static List demoTestScriptNamesOnDisk() { + Files.list(demoDir()).withCloseable { stream -> + stream.map { it.fileName.toString() }.collect(Collectors.toList()) + } + } + + private static final class ProcessResult { + final boolean finished + final int exitCode + final String output + + ProcessResult(boolean finished, int exitCode, String output) { + this.finished = finished + this.exitCode = exitCode + this.output = output + } + } + + /** + * Runs processBuilder to completion (or forcibly kills it past timeoutSeconds), reading its + * output concurrently rather than before the bounded wait. process.inputStream.text (or + * .eachLine on the main thread) blocks until the stream closes, so a hung subprocess would + * hang there even before any waitFor() timeout is reached - the read has to happen on a + * separate thread that runs alongside the bounded wait, not sequentially before it, or the + * timeout is decorative. Uses StringBuffer (not StringBuilder): on the timeout path the + * reader thread may still be writing when reader.join(5000) returns without the thread + * having terminated, so the subsequent toString() read has no happens-before guarantee from + * join() alone and needs the reader's own internal synchronization instead. + */ + private static ProcessResult runBounded(ProcessBuilder processBuilder, long timeoutSeconds) { + Process process = processBuilder.start() + StringBuffer outputBuffer = new StringBuffer() + Thread reader = new Thread({ -> + process.inputStream.eachLine { line -> outputBuffer.append(line).append('\n') } + } as Runnable) + reader.daemon = true + reader.start() + + boolean finished = process.waitFor(timeoutSeconds, TimeUnit.SECONDS) + if (!finished) { + process.destroyForcibly() + process.waitFor(5, TimeUnit.SECONDS) + } + reader.join(5000) + int exitCode = finished ? process.exitValue() : -1 + new ProcessResult(finished, exitCode, outputBuffer.toString()) + } +} diff --git a/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy b/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy index d55f49a..c8cccd5 100644 --- a/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy @@ -97,7 +97,9 @@ class LcaScriptSpec extends Specification { Files.writeString(libDir.resolve("local-coding-assistant-1.0.0-exec.jar"), "jar") Path ollamaLog = tempDir.resolve("ollama.log") Path javaLog = tempDir.resolve("java.log") - writeStubOllama(binDir) + Path ollamaState = tempDir.resolve("ollama-state.txt") + Files.writeString(ollamaState, "other-model:1.0\tid-seed\n") + writeStubOllama(binDir, ollamaState) writeStubJava(binDir) when: @@ -107,7 +109,6 @@ class LcaScriptSpec extends Specification { [ HOME: homeDir.toString(), PATH: binDir.toString() + File.pathSeparator + System.getenv("PATH"), - LCA_OLLAMA_LIST: "other-model:1.0", LCA_OLLAMA_LOG: ollamaLog.toString(), LCA_JAVA_LOG: javaLog.toString() ] @@ -116,17 +117,205 @@ class LcaScriptSpec extends Specification { then: result.exitCode == 0 def log = Files.readString(ollamaLog) - log.contains("pull qwen3.6:35b-a3b") + log.contains("pull qwen3.8:27b") log.contains("pull gpt-oss:20b") - log.contains("create qwen3.6-128k") + log.contains("pull nomic-embed-text:latest") + log.contains("create qwen3.8-192k") log.contains("create gpt-oss-64k") - log.contains("create qwen3.6-review") + log.contains("create qwen3.8-review") Files.exists(javaLog) + def javaEnv = Files.readString(javaLog) + javaEnv.contains("LCA_CHAT_MODEL=qwen3.8-192k:latest") + javaEnv.contains("LCA_FALLBACK_MODEL=gpt-oss-64k:latest") + javaEnv.contains("LCA_EMBEDDING_MODEL=nomic-embed-text:latest") + javaEnv.contains("LCA_REVIEW_MODEL=qwen3.8-review:latest") + javaEnv.contains("LCA_DEFAULT_CONTEXT_WINDOW=131072") + // num_batch/num_gpu are performance tuning for the qwen3.8-based models (chat + + // review), not the gpt-oss fallback. + modelfileSectionFor(log, "qwen3.8-192k").contains("PARAMETER num_batch 2048") + modelfileSectionFor(log, "qwen3.8-192k").contains("PARAMETER num_gpu 99") + modelfileSectionFor(log, "qwen3.8-review").contains("PARAMETER num_batch 2048") + modelfileSectionFor(log, "qwen3.8-review").contains("PARAMETER num_gpu 99") + !modelfileSectionFor(log, "gpt-oss-64k").contains("PARAMETER num_batch") + !modelfileSectionFor(log, "gpt-oss-64k").contains("PARAMETER num_gpu") where: scriptName << scriptNames() } + def "run rebuilds a custom model whose base model changed, even though it already exists"() { + given: + // Simulates switching a base model's backend (e.g. mlx -> llama.cpp): the custom model + // already exists under the same name, but its recorded base-model id is stale, so a naive + // "does a model with this name exist?" check must not be enough to skip rebuilding it. + Path scriptPath = projectRoot().resolve("src/main/bin/lca") + Path homeDir = tempDir.resolve("home-rebuild") + Path binDir = tempDir.resolve("bin-rebuild") + Files.createDirectories(binDir) + Path libDir = homeDir.resolve(".local").resolve("lib") + Files.createDirectories(libDir) + Files.writeString(libDir.resolve("local-coding-assistant-1.0.0-exec.jar"), "jar") + Path ollamaLog = tempDir.resolve("rebuild-ollama.log") + Path javaLog = tempDir.resolve("rebuild-java.log") + Path ollamaState = tempDir.resolve("rebuild-ollama-state.txt") + Files.writeString(ollamaState, "qwen3.8:27b\tnew-llamacpp-id\nqwen3.8-192k:latest\tstale-custom-id\n") + Path modelStateDir = homeDir.resolve(".lca").resolve("model_state") + Files.createDirectories(modelStateDir) + Files.writeString(modelStateDir.resolve("qwen3.8-192k.id"), "old-mlx-id") + writeStubOllama(binDir, ollamaState) + writeStubJava(binDir) + + when: + def result = runScript( + scriptPath, + [], + [ + HOME: homeDir.toString(), + PATH: binDir.toString() + File.pathSeparator + System.getenv("PATH"), + LCA_OLLAMA_LOG: ollamaLog.toString(), + LCA_JAVA_LOG: javaLog.toString() + ] + ) + + then: + result.exitCode == 0 + result.output.contains("Rebuilding qwen3.8-192k") + !result.output.contains("qwen3.8-192k custom model already exists") + Files.readString(ollamaLog).contains("create qwen3.8-192k") + } + + def "run rebuilds a custom model when only its context or extra params changed, base model id unchanged"() { + given: + // Reproduces a real regression: bumping REVIEW_CONTEXT/QWEN_EXTRA_PARAMS in lca without + // the underlying base model itself changing must still trigger a rebuild - a signature + // that only tracks the base model's id would say "up to date" and silently keep serving + // the stale Modelfile (missing the new num_batch/num_gpu tuning). + Path scriptPath = projectRoot().resolve("src/main/bin/lca") + Path homeDir = tempDir.resolve("home-params-changed") + Path binDir = tempDir.resolve("bin-params-changed") + Files.createDirectories(binDir) + Path libDir = homeDir.resolve(".local").resolve("lib") + Files.createDirectories(libDir) + Files.writeString(libDir.resolve("local-coding-assistant-1.0.0-exec.jar"), "jar") + Path ollamaLog = tempDir.resolve("params-ollama.log") + Path javaLog = tempDir.resolve("params-java.log") + Path ollamaState = tempDir.resolve("params-ollama-state.txt") + Files.writeString(ollamaState, "qwen3.8:27b\tsame-base-id\nqwen3.8-review:latest\texisting-custom-id\n") + Path modelStateDir = homeDir.resolve(".lca").resolve("model_state") + Files.createDirectories(modelStateDir) + // Same base id as lca will report now, but recorded against the pre-tuning signature + // (empty extra params) - i.e. the base model itself never changed. + Files.writeString(modelStateDir.resolve("qwen3.8-review.id"), "same-base-id|131072|") + writeStubOllama(binDir, ollamaState) + writeStubJava(binDir) + + when: + def result = runScript( + scriptPath, + [], + [ + HOME: homeDir.toString(), + PATH: binDir.toString() + File.pathSeparator + System.getenv("PATH"), + LCA_OLLAMA_LOG: ollamaLog.toString(), + LCA_JAVA_LOG: javaLog.toString() + ] + ) + + then: + result.exitCode == 0 + result.output.contains("Rebuilding qwen3.8-review") + !result.output.contains("qwen3.8-review is up to date") + modelfileSectionFor(Files.readString(ollamaLog), "qwen3.8-review").contains("PARAMETER num_gpu 99") + } + + def "models.sh derives its model list from lca and installs the same models"() { + given: + Path scriptPath = projectRoot().resolve("models.sh") + Path homeDir = tempDir.resolve("home-models-sh") + Path binDir = tempDir.resolve("bin-models-sh") + Files.createDirectories(binDir) + Path ollamaLog = tempDir.resolve("models-sh-ollama.log") + Path ollamaState = tempDir.resolve("models-sh-ollama-state.txt") + Files.writeString(ollamaState, "other-model:1.0\tid-seed\n") + writeStubOllama(binDir, ollamaState) + + when: + def result = runScript( + scriptPath, + [], + [ + HOME: homeDir.toString(), + PATH: binDir.toString() + File.pathSeparator + System.getenv("PATH"), + LCA_OLLAMA_LOG: ollamaLog.toString() + ] + ) + + then: + result.exitCode == 0 + def log = Files.readString(ollamaLog) + log.contains("pull qwen3.8:27b") + log.contains("pull gpt-oss:20b") + log.contains("pull nomic-embed-text:latest") + log.contains("create qwen3.8-192k") + log.contains("create gpt-oss-64k") + log.contains("create qwen3.8-review") + modelfileSectionFor(log, "qwen3.8-192k").contains("PARAMETER num_batch 2048") + modelfileSectionFor(log, "qwen3.8-192k").contains("PARAMETER num_gpu 99") + modelfileSectionFor(log, "qwen3.8-review").contains("PARAMETER num_batch 2048") + modelfileSectionFor(log, "qwen3.8-review").contains("PARAMETER num_gpu 99") + !modelfileSectionFor(log, "gpt-oss-64k").contains("PARAMETER num_batch") + !modelfileSectionFor(log, "gpt-oss-64k").contains("PARAMETER num_gpu") + } + + def "models.sh rebuilds a custom model when only its context or extra params changed, base model id unchanged"() { + given: + // Mirrors lca's own regression test: models.sh's createCustomModel must fingerprint the + // full desired Modelfile recipe (base id + context + extra params), not just whether a + // model with this name already exists - the exact gap that let a stale custom model keep + // serving silently after a context/parameter-only edit. + Path scriptPath = projectRoot().resolve("models.sh") + Path homeDir = tempDir.resolve("home-models-sh-rebuild") + Path binDir = tempDir.resolve("bin-models-sh-rebuild") + Files.createDirectories(binDir) + Path ollamaLog = tempDir.resolve("models-sh-rebuild-ollama.log") + Path ollamaState = tempDir.resolve("models-sh-rebuild-ollama-state.txt") + Files.writeString(ollamaState, "qwen3.8:27b\tsame-base-id\nqwen3.8-review:latest\texisting-custom-id\n") + Path modelStateDir = homeDir.resolve(".lca").resolve("model_state") + Files.createDirectories(modelStateDir) + // Same base id the stub will report now, but recorded against the pre-tuning signature + // (empty extra params) - i.e. the base model itself never changed. + Files.writeString(modelStateDir.resolve("qwen3.8-review.id"), "same-base-id|131072|") + writeStubOllama(binDir, ollamaState) + + when: + def result = runScript( + scriptPath, + [], + [ + HOME: homeDir.toString(), + PATH: binDir.toString() + File.pathSeparator + System.getenv("PATH"), + LCA_OLLAMA_LOG: ollamaLog.toString() + ] + ) + + then: + result.exitCode == 0 + result.output.contains("Rebuilding qwen3.8-review") + !result.output.contains("qwen3.8-review is up to date") + modelfileSectionFor(Files.readString(ollamaLog), "qwen3.8-review").contains("PARAMETER num_gpu 99") + } + + private static String modelfileSectionFor(String log, String modelName) { + String startMarker = "--- modelfile:${modelName} ---" + String endMarker = "--- end modelfile:${modelName} ---" + int start = log.indexOf(startMarker) + if (start < 0) { + return "" + } + int end = log.indexOf(endMarker, start) + end < 0 ? log.substring(start) : log.substring(start, end) + } + private static Path projectRoot() { Paths.get("").toAbsolutePath().normalize() } @@ -191,25 +380,61 @@ exit 1 curlPath.toFile().setExecutable(true) } - private static void writeStubOllama(Path binDir) { + /** + * A stateful ollama stub: {@code list} reflects a backing state file that {@code pull} and + * {@code create} append to (each with a synthetic, resolvable model id) and {@code rm} removes + * from - so {@code get_model_id} in lca resolves realistically after a pull/create, letting + * tests exercise rebuild_custom_model_if_changed's id-comparison logic, not just existence. + * Pre-seed {@code stateFile} (tab-separated "name\tid" lines) to simulate models that are + * already present before the script runs. + */ + private static void writeStubOllama(Path binDir, Path stateFile) { + if (!Files.exists(stateFile)) { + Files.writeString(stateFile, "") + } Path ollamaPath = binDir.resolve("ollama") Files.writeString( ollamaPath, """#!/usr/bin/env bash set -euo pipefail +STATE_FILE="${stateFile}" + command="\${1:-}" case "\$command" in list) if [ -n "\${LCA_OLLAMA_LOG:-}" ]; then echo "list" >> "\$LCA_OLLAMA_LOG" fi - printf '%s\n' "\${LCA_OLLAMA_LIST:-}" + cat "\$STATE_FILE" 2>/dev/null || true ;; pull) + model="\${2:-}" + if [ -n "\${LCA_OLLAMA_LOG:-}" ]; then + echo "pull \$model" >> "\$LCA_OLLAMA_LOG" + fi + printf '%s\\tid-%s-%s\\n' "\$model" "\$\$" "\$RANDOM" >> "\$STATE_FILE" + ;; + create) + name="\${2:-}" + modelfile_path="\${4:-}" + if [ -n "\${LCA_OLLAMA_LOG:-}" ]; then + echo "create \$*" >> "\$LCA_OLLAMA_LOG" + if [ -n "\$modelfile_path" ] && [ -f "\$modelfile_path" ]; then + echo "--- modelfile:\$name ---" >> "\$LCA_OLLAMA_LOG" + cat "\$modelfile_path" >> "\$LCA_OLLAMA_LOG" + echo "--- end modelfile:\$name ---" >> "\$LCA_OLLAMA_LOG" + fi + fi + printf '%s:latest\\tid-%s-%s\\n' "\$name" "\$\$" "\$RANDOM" >> "\$STATE_FILE" + ;; + rm) + name="\${2:-}" if [ -n "\${LCA_OLLAMA_LOG:-}" ]; then - echo "pull \${2:-}" >> "\$LCA_OLLAMA_LOG" + echo "rm \$name" >> "\$LCA_OLLAMA_LOG" fi + grep -v "^\${name}[[:space:]]" "\$STATE_FILE" > "\${STATE_FILE}.tmp" 2>/dev/null || true + mv "\${STATE_FILE}.tmp" "\$STATE_FILE" 2>/dev/null || true ;; *) if [ -n "\${LCA_OLLAMA_LOG:-}" ]; then @@ -233,6 +458,7 @@ set -euo pipefail if [ -n "\${LCA_JAVA_LOG:-}" ]; then echo "\$*" >> "\$LCA_JAVA_LOG" + env | grep '^LCA_[A-Z_]*=' >> "\$LCA_JAVA_LOG" || true fi exit 0 diff --git a/src/test/groovy/se/alipsa/lca/shell/BatchTestModelConfiguration.groovy b/src/test/groovy/se/alipsa/lca/shell/BatchTestModelConfiguration.groovy index 50a44d0..7de2a77 100644 --- a/src/test/groovy/se/alipsa/lca/shell/BatchTestModelConfiguration.groovy +++ b/src/test/groovy/se/alipsa/lca/shell/BatchTestModelConfiguration.groovy @@ -33,7 +33,7 @@ class BatchTestModelConfiguration { @Bean LlmService batchTestLlm(ChatModel chatModel) { - new SpringAiLlmService("qwen3.6:35b-a3b", "test", chatModel) + new SpringAiLlmService("qwen3.8:27b", "test", chatModel) } @Bean diff --git a/src/test/groovy/se/alipsa/lca/shell/OllamaLlmIntegrationSpec.groovy b/src/test/groovy/se/alipsa/lca/shell/OllamaLlmIntegrationSpec.groovy index 898e8d9..debe910 100644 --- a/src/test/groovy/se/alipsa/lca/shell/OllamaLlmIntegrationSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/shell/OllamaLlmIntegrationSpec.groovy @@ -18,7 +18,7 @@ import java.util.concurrent.TimeUnit * * Requirements: * - Ollama must be running at http://localhost:11434 - * - Model qwen3.6:35b-a3b must be pulled (or fallback gpt-oss:20b) + * - Model qwen3.8:27b must be pulled (or fallback gpt-oss:20b) * - System property runSlowIntegrationTests must be set to true * * Tests will be skipped if: diff --git a/src/test/groovy/se/alipsa/lca/shell/ShellCommandsSpec.groovy b/src/test/groovy/se/alipsa/lca/shell/ShellCommandsSpec.groovy index fa96641..e39604b 100644 --- a/src/test/groovy/se/alipsa/lca/shell/ShellCommandsSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/shell/ShellCommandsSpec.groovy @@ -1682,6 +1682,85 @@ class ShellCommandsSpec extends Specification { cmds.health().contains("unreachable") } + def "benchmark reports tokens per second and context length for the resolved model"() { + given: + ModelRegistry.BenchmarkResult result = new ModelRegistry.BenchmarkResult( + "qwen3.8-review", 22391331208L, 7425792167L, 19, 774138000L, 190, 14189978000L) + ModelRegistry registry = Stub() { + checkHealth() >> new ModelRegistry.Health(true, "ok") + listModels() >> ["qwen3.8-review"] + benchmark("qwen3.8-review", _, 200) >> result + contextLength("qwen3.8-review") >> 131072 + } + ShellCommands cmds = benchmarkCommandsFor(registry) + + when: + String out = cmds.benchmark("qwen3.8-review", null, null, 200, "default") + + then: + out.contains("qwen3.8-review") + out.contains("131072") + out.contains("19") + out.contains("190") + } + + def "benchmark reports unreachable when Ollama is down"() { + given: + ModelRegistry registry = Stub() { + checkHealth() >> new ModelRegistry.Health(false, "connection refused") + getBaseUrl() >> "http://localhost:11434" + } + ShellCommands cmds = benchmarkCommandsFor(registry) + + expect: + cmds.benchmark(null, null, null, 200, "default").contains("unreachable") + } + + def "benchmark rejects an unknown model"() { + given: + ModelRegistry registry = Stub() { + checkHealth() >> new ModelRegistry.Health(true, "ok") + listModels() >> ["known-model"] + } + ShellCommands cmds = benchmarkCommandsFor(registry) + + when: + String out = cmds.benchmark("unknown-model", null, null, 200, "default") + + then: + out.contains("not found") + out.contains("known-model") + } + + def "benchmark reads the prompt from --prompt-file"() { + given: + ModelRegistry.BenchmarkResult result = new ModelRegistry.BenchmarkResult("m", 1L, 1L, 1, 1L, 1, 1L) + ModelRegistry registry = Stub() { + checkHealth() >> new ModelRegistry.Health(true, "ok") + listModels() >> ["m"] + benchmark("m", "file contents", 200) >> result + contextLength("m") >> null + } + fileEditingTool.readFile("prompt.txt") >> "file contents" + ShellCommands cmds = benchmarkCommandsFor(registry) + + expect: + cmds.benchmark("m", null, "prompt.txt", 200, "default").contains("Model: m") + } + + def "benchmark surfaces a missing prompt file"() { + given: + ModelRegistry registry = Stub() { + checkHealth() >> new ModelRegistry.Health(true, "ok") + listModels() >> ["m"] + } + fileEditingTool.readFile("missing.txt") >> { throw new IllegalArgumentException("File missing.txt does not exist") } + ShellCommands cmds = benchmarkCommandsFor(registry) + + expect: + cmds.benchmark("m", null, "missing.txt", 200, "default").contains("does not exist") + } + def "version returns resolved version"() { given: ShellCommands versioned = new ShellCommands( @@ -2642,6 +2721,43 @@ class ShellCommandsSpec extends Specification { !response.contains("not found in the project") } + private ShellCommands benchmarkCommandsFor(ModelRegistry registry) { + new ShellCommands( + agent, + ai, + sessionState, + editorLauncher, + fileEditingTool, + Mock(se.alipsa.lca.tools.ToolCallParser), + gitTool, + Stub(CodeSearchTool), + new ContextPacker(), + new ContextBudgetManager(10000, 0, new TokenEstimator(), 2, -1), + commandRunner, + commandPolicy, + registry, + agentPlatform, + contextRepository, + tempDir.resolve("benchmark.log").toString(), + null, + null, + shellSettings, + intentRoutingState, + intentRoutingSettings + , + Mock(se.alipsa.lca.validation.RequestValidator), + Mock(se.alipsa.lca.validation.ClarificationDialog), + null, + null, + null, + null, + contextCompactor, + 80000, + 30000, + null + ) + } + private ShellCommands commitCommandsFor(GitTool repoGit) { new ShellCommands( agent, diff --git a/src/test/groovy/se/alipsa/lca/tools/ModelRegistrySpec.groovy b/src/test/groovy/se/alipsa/lca/tools/ModelRegistrySpec.groovy index aa7d3d9..bb1f1ad 100644 --- a/src/test/groovy/se/alipsa/lca/tools/ModelRegistrySpec.groovy +++ b/src/test/groovy/se/alipsa/lca/tools/ModelRegistrySpec.groovy @@ -102,11 +102,11 @@ class ModelRegistrySpec extends Specification { def "contextLength reads model_info context_length"() { given: - String json = '{"model_info": {"qwen3.architecture": "qwen3", "qwen3.context_length": 131072}}' + String json = '{"model_info": {"qwen3.architecture": "qwen3", "qwen3.context_length": 196608}}' ModelRegistry registry = new ShowRegistry(200, json) expect: - registry.contextLength("qwen3.6-128k:latest") == 131072 + registry.contextLength("qwen3.8-192k:latest") == 196608 } def "contextLength returns null when not reported"() { @@ -148,6 +148,51 @@ class ModelRegistrySpec extends Specification { ["qwen3.architecture": "qwen3", "qwen3.context_length": 131072] as Map) == 131072 } + def "benchmark computes prompt and eval tokens per second from Ollama's generate response"() { + given: + String json = ''' + {"total_duration": 22391331208, "load_duration": 7425792167, + "prompt_eval_count": 19, "prompt_eval_duration": 774138000, + "eval_count": 190, "eval_duration": 14189978000} + ''' + ModelRegistry registry = new GenerateRegistry(200, json) + + when: + ModelRegistry.BenchmarkResult result = registry.benchmark("qwen3.8-review", "hello", 200) + + then: + result.promptEvalCount == 19 + result.evalCount == 190 + Math.abs(result.promptTokensPerSecond - 24.54d) < 0.1d + Math.abs(result.evalTokensPerSecond - 13.39d) < 0.1d + } + + def "benchmark returns zero rate when duration is zero"() { + given: + ModelRegistry registry = new GenerateRegistry(200, + '{"eval_count": 5, "eval_duration": 0, "prompt_eval_count": 3, "prompt_eval_duration": 0}') + + when: + ModelRegistry.BenchmarkResult result = registry.benchmark("m", "hi", 10) + + then: + result.evalTokensPerSecond == 0d + result.promptTokensPerSecond == 0d + } + + def "benchmark throws on a non-2xx response"() { + given: + ModelRegistry registry = new GenerateRegistry(500, "model not found") + + when: + registry.benchmark("missing-model", "hi", 10) + + then: + IOException e = thrown(IOException) + e.message.contains("500") + e.message.contains("model not found") + } + def "contextLengthFromModelInfo falls back to the first match when the architecture key doesn't resolve"() { expect: ModelRegistry.contextLengthFromModelInfo( @@ -180,8 +225,8 @@ class ModelRegistrySpec extends Specification { def "isRemote reflects the configured base URL's host"() { expect: - !new ModelRegistry("http://localhost:11434", 1000L, 30000L, 5000L, HttpClient.newHttpClient()).isRemote() - new ModelRegistry("http://ollama.example.com:11434", 1000L, 30000L, 5000L, HttpClient.newHttpClient()).isRemote() + !new ModelRegistry("http://localhost:11434", 1000L, 30000L, 5000L, 30000L, HttpClient.newHttpClient()).isRemote() + new ModelRegistry("http://ollama.example.com:11434", 1000L, 30000L, 5000L, 30000L, HttpClient.newHttpClient()).isRemote() } def "loadedModels parses name/size/size_vram, falling back to the model key for the name"() { @@ -189,7 +234,7 @@ class ModelRegistrySpec extends Specification { ModelRegistry registry = new PsRegistry(200, ''' {"models": [ {"name": "mistral:latest", "size": 5137025024, "size_vram": 5137025024}, - {"model": "qwen3.6:latest", "size": 8000000000} + {"model": "qwen3.8:latest", "size": 8000000000} ]} ''') @@ -201,7 +246,7 @@ class ModelRegistrySpec extends Specification { loaded[0].name == "mistral:latest" loaded[0].size == 5137025024L loaded[0].sizeVram == 5137025024L - loaded[1].name == "qwen3.6:latest" + loaded[1].name == "qwen3.8:latest" loaded[1].size == 8000000000L loaded[1].sizeVram == 0L } @@ -261,7 +306,7 @@ class ModelRegistrySpec extends Specification { private final String body ShowRegistry(int status, String body) { - super("http://localhost:11434", 1000L, 30000L, 5000L, HttpClient.newHttpClient()) + super("http://localhost:11434", 1000L, 30000L, 5000L, 30000L, HttpClient.newHttpClient()) this.status = status this.body = body } @@ -272,12 +317,28 @@ class ModelRegistrySpec extends Specification { } } + private static class GenerateRegistry extends ModelRegistry { + private final int status + private final String body + + GenerateRegistry(int status, String body) { + super("http://localhost:11434", 1000L, 30000L, 5000L, 30000L, HttpClient.newHttpClient()) + this.status = status + this.body = body + } + + @Override + protected HttpResponse fetchGenerate(String model, String prompt, int maxTokens) throws Exception { + [statusCode: { -> status }, body: { -> body }] as HttpResponse + } + } + private static class PsRegistry extends ModelRegistry { private final int status private final String body PsRegistry(int status, String body) { - super("http://localhost:11434", 1000L, 30000L, 5000L, HttpClient.newHttpClient()) + super("http://localhost:11434", 1000L, 30000L, 5000L, 30000L, HttpClient.newHttpClient()) this.status = status this.body = body } @@ -293,7 +354,7 @@ class ModelRegistrySpec extends Specification { private final List models FakeRegistry(boolean reachable, List models) { - super("http://localhost:11434", 1000L, 30000L, 5000L, HttpClient.newHttpClient()) + super("http://localhost:11434", 1000L, 30000L, 5000L, 30000L, HttpClient.newHttpClient()) this.reachable = reachable this.models = models } @@ -317,7 +378,7 @@ class ModelRegistrySpec extends Specification { private final boolean throwOnTags ErrorRegistry(boolean reachable, boolean throwOnTags) { - super("http://localhost:11434", 1000L, 30000L, 5000L, HttpClient.newHttpClient()) + super("http://localhost:11434", 1000L, 30000L, 5000L, 30000L, HttpClient.newHttpClient()) this.reachable = reachable this.throwOnTags = throwOnTags } diff --git a/src/test/resources/application-batch-test.properties b/src/test/resources/application-batch-test.properties index ecf817b..77041fa 100644 --- a/src/test/resources/application-batch-test.properties +++ b/src/test/resources/application-batch-test.properties @@ -1,11 +1,11 @@ # Override custom model references with base models for testing. -# The custom large-context models (qwen3.6-128k / gpt-oss-64k) are created by the +# The custom large-context models (qwen3.8-192k / gpt-oss-64k) are created by the # lca script at runtime and may not exist in the test environment. -embabel.models.default-llm=qwen3.6:35b-a3b -embabel.models.llms.best=qwen3.6:35b-a3b +embabel.models.default-llm=qwen3.8:27b +embabel.models.llms.best=qwen3.8:27b embabel.models.llms.cheapest=gpt-oss:20b embabel.agent-platform.ranking.llm=gpt-oss:20b -assistant.llm.model=qwen3.6:35b-a3b +assistant.llm.model=qwen3.8:27b assistant.llm.fallback-model=gpt-oss:20b assistant.intent.model=gpt-oss:20b -assistant.intent.fallback-model=qwen3.6:35b-a3b +assistant.intent.fallback-model=qwen3.8:27b