From d4c070b6de17266a6ac4d466b8852d91f29708a8 Mon Sep 17 00:00:00 2001 From: pernyf Date: Sun, 16 Aug 2026 14:56:26 +0200 Subject: [PATCH 01/18] feat(demo): switch openCodeMlx default model to Qwen3.8-27B-8bit via mlx-vlm Qwen3.8-27B-8bit is a vision-language checkpoint that mlx_lm.server can't load (unsupported architecture), so serving it requires mlx-vlm's own OpenAI-compatible server instead. Auto-detect the backend per model from its downloaded config.json (vision_config presence) so future MLX_MODEL swaps between text-only and vision-capable checkpoints keep working with just the one-line change the script already supports. Co-Authored-By: Claude Sonnet 5 --- demo/openCodeMlx | 114 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 100 insertions(+), 14 deletions(-) diff --git a/demo/openCodeMlx b/demo/openCodeMlx index d772370..48c81bf 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -6,8 +6,12 @@ ############################################################ # --- 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-8bit}" +#MLX_MODEL="${MLX_MODEL:-mlx-community/Qwen3-Coder-30B-A3B-Instruct-8bit}" #MLX_MODEL="${MLX_MODEL:-mlx-community/Qwen3-Coder-Next-4bit}" # Small/fast model used for lightweight background tasks (e.g. session title @@ -126,6 +130,35 @@ 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). Deliberately does not pin +# transformers here: mlx-lm's own pin above already constrains the shared +# venv, and guessing a separate mlx-vlm-specific range without evidence +# risks a silent version conflict between the two packages. +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; 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; then + echo "Warning: mlx-vlm update check failed, continuing with existing installation." >&2 + fi + return 0 +} + # _download_via_huggingface _download_via_huggingface() { local model_id="$1" local_dir="$2" @@ -166,6 +199,29 @@ _download_via() { esac } +# 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 @@ -348,6 +404,14 @@ 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 + 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 +452,32 @@ 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}" + + 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. + 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 \ + >> "$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)..." @@ -471,11 +551,17 @@ 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 + MAIN_CACHE_LIMIT="$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"; 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 From 8cffc9d46482be518a339b7dc339f63706a13e6e Mon Sep 17 00:00:00 2001 From: pernyf Date: Sun, 16 Aug 2026 20:04:18 +0200 Subject: [PATCH 02/18] feat(openCodeMlx): speculative decoding + bind downloads to en7 Switch to mlx-community/Qwen3.8-27B-4bit paired with the Qwen3.8-27B-MTP-4bit draft head via mlx_vlm.server's --draft-model/--draft-kind mtp, since the 8-bit model alone was too slow for practical use. Verified live: 17.8 tok/s decode (up from ~12 tok/s baseline) with a ~77% draft-token acceptance rate. Also bind model downloads to a specific network interface (MLX_DOWNLOAD_INTERFACE, default en7) via a socket.socket subclass in the huggingface_hub/modelscope download snippets, since GlobalProtect VPN was found to reset every connection to huggingface.co outright. Disables hf_xet when binding is active, since its separate Rust networking stack bypasses the socket patch and was observed to hang indefinitely. Co-Authored-By: Claude Sonnet 5 --- demo/openCodeMlx | 106 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 101 insertions(+), 5 deletions(-) diff --git a/demo/openCodeMlx b/demo/openCodeMlx index 48c81bf..04f6219 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -10,16 +10,37 @@ # 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-8bit}" +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. +MLX_DRAFT_MODEL="${MLX_DRAFT_MODEL:-mlx-community/Qwen3.8-27B-MTP-4bit}" + # 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. Defaults to en7 (this +# author's real Ethernet interface): GlobalProtect VPN was found to reset +# every connection to huggingface.co outright when it's the default route +# (utun0), while binding to en7 gets a clean, fast response - so this +# bypasses the VPN specifically for model downloads. Set to "" to disable +# and use whatever the default route is; if the named interface doesn't +# exist/resolve on your machine this warns and falls back to the default +# route automatically, so it's safe to leave as-is elsewhere. +MLX_DOWNLOAD_INTERFACE="${MLX_DOWNLOAD_INTERFACE:-en7}" + # 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 @@ -159,11 +180,70 @@ ensure_mlx_vlm_current() { 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 @@ -179,6 +259,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 @@ -314,7 +395,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 @@ -389,6 +470,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, @@ -412,6 +495,14 @@ main() { fi fi + LOCAL_DRAFT_MODEL_DIR="" + if [[ -n "${MLX_DRAFT_MODEL:-}" ]]; then + 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 + 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 @@ -452,7 +543,7 @@ 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" cache_limit="$9" backend="${10:-mlx_lm}" + 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 @@ -461,6 +552,10 @@ EOF # 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 \ @@ -468,6 +563,7 @@ EOF --prefill-step-size "$prefill_step_size" \ --max-kv-size "$cache_limit" \ --trust-remote-code \ + "${draft_args[@]}" \ >> "$log_file" 2>&1 & else mlx_lm.server \ @@ -557,7 +653,7 @@ EOF 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"; then + 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 From bf5e511c2929a42fa3ca0d7ef141e74def314ec3 Mon Sep 17 00:00:00 2001 From: pernyf Date: Sun, 16 Aug 2026 20:09:55 +0200 Subject: [PATCH 03/18] fix(openCodeMlx): make download interface binding opt-in, not en7 by default This is an open-source repo; defaulting to a specific machine's network interface name doesn't generalize. MLX_DOWNLOAD_INTERFACE now defaults to empty (default route), and users can opt in with their own interface name if they hit VPN issues like the GlobalProtect one described in the comment. Co-Authored-By: Claude Sonnet 5 --- demo/openCodeMlx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/demo/openCodeMlx b/demo/openCodeMlx index 04f6219..e3ace42 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -31,15 +31,15 @@ MLX_DRAFT_MODEL="${MLX_DRAFT_MODEL:-mlx-community/Qwen3.8-27B-MTP-4bit}" # 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. Defaults to en7 (this -# author's real Ethernet interface): GlobalProtect VPN was found to reset -# every connection to huggingface.co outright when it's the default route -# (utun0), while binding to en7 gets a clean, fast response - so this -# bypasses the VPN specifically for model downloads. Set to "" to disable -# and use whatever the default route is; if the named interface doesn't -# exist/resolve on your machine this warns and falls back to the default -# route automatically, so it's safe to leave as-is elsewhere. -MLX_DOWNLOAD_INTERFACE="${MLX_DOWNLOAD_INTERFACE:-en7}" +# 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. From bffd87f85cae0f56811427b825d6a562c9655077 Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 15:41:49 +0200 Subject: [PATCH 04/18] feat(models): consolidate model config, tune GPU offload, add /benchmark MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates scattered Ollama model config (names, context sizes, custom model recipes) into src/main/bin/lca as the single canonical source, with models.sh deriving from it and application.properties picking it up via LCA_* env vars lca exports at launch. Switches the chat/review models from the mlx variant to llama.cpp, raises the review model's context to 128k, and applies num_gpu/num_batch tuning to the main model for full GPU offload on Apple Silicon. Fixes rebuild_custom_model_if_changed() to fingerprint the full desired Modelfile recipe (base id + context + extra params), not just the base model's id — previously a context/parameter-only change went undetected and a stale custom model silently kept running. Adds a /benchmark command (ModelRegistry.benchmark() calling Ollama's /api/generate directly) to measure real tokens/sec for a given model, optionally against a real file's contents via --prompt-file, so future model/config changes can be measured instead of guessed at. Also fixes two intent-routing bugs found while wiring /benchmark in: literal, well-formed slash commands (e.g. "/benchmark --model x") no longer risk being misrouted by the LLM intent classifier — they're dispatched directly when CommandExecutor already recognizes them. And COMMAND_PATTERN's \w+ was truncating hyphenated command names at the first hyphen, so /git-apply and the entirely-unwired /git-push could never actually be dispatched by name; both are fixed and wired up. Co-Authored-By: Claude Sonnet 5 --- models.sh | 81 ++++--- src/main/bin/lca | 121 ++++++----- .../lca/agent/CodingAssistantAgent.groovy | 2 +- .../se/alipsa/lca/gui/ContextEstimator.groovy | 6 +- .../lca/intent/IntentRouterAgent.groovy | 2 +- .../alipsa/lca/memory/MemorySettings.groovy | 2 + .../se/alipsa/lca/repl/CommandExecutor.groovy | 54 ++++- .../se/alipsa/lca/repl/JLineRepl.groovy | 8 + .../se/alipsa/lca/shell/SessionState.groovy | 2 +- .../se/alipsa/lca/shell/ShellCommands.groovy | 65 ++++++ .../se/alipsa/lca/team/TeamSettings.groovy | 8 +- .../se/alipsa/lca/tools/ModelRegistry.groovy | 61 ++++++ src/main/resources/application.properties | 51 +++-- .../lca/repl/CommandExecutorSpec.groovy | 59 +++++ .../se/alipsa/lca/repl/JLineReplSpec.groovy | 22 ++ .../alipsa/lca/scripts/LcaScriptSpec.groovy | 202 +++++++++++++++++- .../shell/BatchTestModelConfiguration.groovy | 2 +- .../lca/shell/OllamaLlmIntegrationSpec.groovy | 2 +- .../alipsa/lca/shell/ShellCommandsSpec.groovy | 116 ++++++++++ .../alipsa/lca/tools/ModelRegistrySpec.groovy | 81 ++++++- .../application-batch-test.properties | 8 +- 21 files changed, 817 insertions(+), 138 deletions(-) diff --git a/models.sh b/models.sh index 0bfd7f9..f575307 100755 --- a/models.sh +++ b/models.sh @@ -1,5 +1,31 @@ #!/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)=' "$LCA_SCRIPT")" + os="" case "$(uname -s)" in Darwin) @@ -57,21 +83,37 @@ createCustomModel() { base_model="$1" custom_name="$2" context_size="$3" + extra_params="${4:-}" echo "Creating custom model $custom_name from $base_model with context size $context_size..." # Check if custom model already exists if ollama list 2>/dev/null | grep -q "^$custom_name"; then - echo "$custom_name already exists." - return + if [ "$force" = true ]; then + echo "$custom_name already exists. Removing before recreating (--force)..." + ollama rm "$custom_name" + else + echo "$custom_name already exists." + return + fi 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" @@ -83,26 +125,13 @@ EOF } # 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..e3ed460 100644 --- a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy +++ b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy @@ -21,7 +21,23 @@ 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" + ) private final ShellCommands shellCommands private final McpCommands mcpCommands @@ -73,6 +89,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 +111,8 @@ class CommandExecutor { return shellCommands.help() case "health": return shellCommands.health() + case "benchmark": + return executeBenchmark(args) case "exit": case "quit": // Trigger system exit @@ -103,6 +123,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 @@ -330,6 +363,25 @@ class CommandExecutor { ) } + private String executeGitPush(String args) { + Map parsed = parseArgs(args) + shellCommands.gitPush( + parseBoolean(parsed.force) ?: false, + parseBoolean(parsed.confirm) ?: true + ) + } + + private String executeBenchmark(String args) { + Map parsed = parseArgs(args) + shellCommands.benchmark( + parsed.model as String, + parsed.prompt as String, + parsed.promptFile as String, + parseInt(parsed.maxTokens) ?: 200, + parsed.session as String ?: "default" + ) + } + private String executeCompact(String args) { Map parsed = parseArgs(args) shellCommands.compact(parsed.session as String ?: "default") 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..1e4b40f 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,70 @@ 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 + ) { + 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("%.2f", result.promptTokensPerSecond)).append(" tok/s)\n") + body.append("Generated tokens: ").append(result.evalCount) + .append(" (").append(String.format("%.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("%.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..db64e02 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 { @@ -46,6 +47,64 @@ class CommandExecutorSpec extends Specification { result == "ran" } + 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" + } + + @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" || false + "/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/LcaScriptSpec.groovy b/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy index d55f49a..5448972 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,165 @@ 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 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, + [], + [ + 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") + } + + 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 +340,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 +418,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..42dd0e6 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 # 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 From 905a01ee79c9b11f2c1bba77fc49b761edd17c30 Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 15:59:29 +0200 Subject: [PATCH 05/18] fix(models,benchmark): address review findings on the config/benchmark PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - models.sh's createCustomModel now fingerprints the same base_id|ctx|extra signature lca's rebuild_custom_model_if_changed uses, sharing its MODEL_STATE_DIR — previously it only checked "does a model with this name exist?", so the exact staleness bug fixed on the lca side was still live when running models.sh directly (a context/param-only edit silently kept serving the old custom model, requiring -f to notice). - models.sh now asserts every variable it reads from lca via grep+eval actually resolved to a non-empty value, failing fast instead of silently proceeding with an empty model name/context if the grep alternation and lca's variable names ever drift apart. - /benchmark --max-tokens 0 no longer silently becomes 200 (Groovy truthiness treats 0 as falsy under "?: 200"); negative/zero values are now rejected with a clear message instead of passing num_predict: -1 through to Ollama unbounded. - /benchmark's tokens/sec output now formats with Locale.ROOT so it's consistent across JVM locales — the point of the numbers is comparing them across config/model changes. - Added a test guarding the known asymmetric drift risk in CommandExecutor's KNOWN_COMMANDS bypass set: every command it claims to recognize is asserted to actually dispatch instead of falling through to the unknown-command fallback. - Fixed a stale qwen3.6-128k reference in application-batch-test.properties' comment (model was renamed to qwen3.8-192k earlier in this branch). Co-Authored-By: Claude Sonnet 5 --- models.sh | 64 +++++++++++++++---- .../se/alipsa/lca/repl/CommandExecutor.groovy | 5 +- .../se/alipsa/lca/shell/ShellCommands.groovy | 9 ++- .../lca/repl/CommandExecutorSpec.groovy | 23 +++++++ .../alipsa/lca/scripts/LcaScriptSpec.groovy | 40 ++++++++++++ .../application-batch-test.properties | 2 +- 6 files changed, 127 insertions(+), 16 deletions(-) diff --git a/models.sh b/models.sh index f575307..6f268ef 100755 --- a/models.sh +++ b/models.sh @@ -24,7 +24,21 @@ 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)=' "$LCA_SCRIPT")" +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 @@ -67,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..." @@ -85,17 +104,38 @@ createCustomModel() { context_size="$3" extra_params="${4:-}" - echo "Creating custom model $custom_name from $base_model with context size $context_size..." + 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 - # Check if custom model already exists - if ollama list 2>/dev/null | grep -q "^$custom_name"; then - if [ "$force" = true ]; then - echo "$custom_name already exists. Removing before recreating (--force)..." - ollama rm "$custom_name" - else - echo "$custom_name already exists." - return - fi + # 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 + + 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 @@ -120,6 +160,8 @@ createCustomModel() { # Clean up rm "$modelfile" + mkdir -p "$MODEL_STATE_DIR" + printf '%s\n' "$desired_signature" > "$state_file" echo "$custom_name created successfully." } diff --git a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy index e3ed460..a2a1353 100644 --- a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy +++ b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy @@ -373,11 +373,14 @@ class CommandExecutor { 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, - parseInt(parsed.maxTokens) ?: 200, + maxTokens != null ? maxTokens : 200, parsed.session as String ?: "default" ) } diff --git a/src/main/groovy/se/alipsa/lca/shell/ShellCommands.groovy b/src/main/groovy/se/alipsa/lca/shell/ShellCommands.groovy index 1e4b40f..7953d59 100644 --- a/src/main/groovy/se/alipsa/lca/shell/ShellCommands.groovy +++ b/src/main/groovy/se/alipsa/lca/shell/ShellCommands.groovy @@ -1342,6 +1342,9 @@ Try: @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}" @@ -1380,16 +1383,16 @@ Try: body.append("Reported context length: ").append(contextLength).append(" tokens\n") } body.append("Prompt tokens: ").append(result.promptEvalCount) - .append(" (").append(String.format("%.2f", result.promptTokensPerSecond)).append(" tok/s)\n") + .append(" (").append(String.format(Locale.ROOT, "%.2f", result.promptTokensPerSecond)).append(" tok/s)\n") body.append("Generated tokens: ").append(result.evalCount) - .append(" (").append(String.format("%.2f", result.evalTokensPerSecond)).append(" tok/s)\n") + .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("%.2fs", nanos / 1_000_000_000d) + String.format(Locale.ROOT, "%.2fs", nanos / 1_000_000_000d) } @ShellMethod( diff --git a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy index db64e02..7839b75 100644 --- a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy @@ -84,6 +84,29 @@ class CommandExecutorSpec extends Specification { 1 * shellCommands.gitPush(false, true) >> "pushed" } + @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" + ] + } + @Unroll def "isKnownCommand('#input') == #expected"() { expect: diff --git a/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy b/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy index 5448972..c8cccd5 100644 --- a/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/scripts/LcaScriptSpec.groovy @@ -231,6 +231,7 @@ class LcaScriptSpec extends Specification { 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") @@ -243,6 +244,7 @@ class LcaScriptSpec extends Specification { scriptPath, [], [ + HOME: homeDir.toString(), PATH: binDir.toString() + File.pathSeparator + System.getenv("PATH"), LCA_OLLAMA_LOG: ollamaLog.toString() ] @@ -265,6 +267,44 @@ class LcaScriptSpec extends Specification { !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} ---" diff --git a/src/test/resources/application-batch-test.properties b/src/test/resources/application-batch-test.properties index 42dd0e6..77041fa 100644 --- a/src/test/resources/application-batch-test.properties +++ b/src/test/resources/application-batch-test.properties @@ -1,5 +1,5 @@ # 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.8:27b embabel.models.llms.best=qwen3.8:27b From bb095dc6c393e498e02f801232cf3ce21181e94a Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:08:33 +0200 Subject: [PATCH 06/18] fix(openCodeMlx): shared-venv transformers pin, draft-model gating/mismatch, stale label - ensure_mlx_vlm_current() now carries the same "transformers>=5.7,<5.13" pin as ensure_mlx_lm_current: both share one venv, so an unconstrained "pip install mlx-vlm" could upgrade transformers past 5.13 to satisfy its own metadata, silently breaking mlx-lm's AutoTokenizer.register call at import time - and mlx_lm.server is still used unconditionally for the small background model regardless of which backend serves the main model. - MLX_DRAFT_MODEL is no longer downloaded when the main model isn't served via mlx_vlm: --draft-model is only ever passed on that path, so gating on MLX_DRAFT_MODEL alone downloaded a multi-GB drafter that start_mlx_server_instance would then silently discard on the mlx_lm path. Warns instead when this happens. - Added a fail-fast quant-suffix check (new _quant_suffix helper) so a MLX_DRAFT_MODEL whose quant level doesn't match MLX_MODEL's is rejected with a clear error before downloading anything, instead of silently serving a mismatched drafter/base-model pair. - --max-kv-size (the mlx_vlm path's closest analogue to --prompt-cache-bytes) is now overridable via MLX_VLM_MAX_KV_SIZE, with the comment now stating plainly that reusing MLX_CONTEXT_LIMIT as a token-count stand-in is an unverified guess at the real per-token KV footprint, not a measured bound. - cleanup()'s stop_and_report label now reflects the actually-detected backend ("mlx_vlm.server (main)" vs "mlx_lm.server (main)") instead of always saying mlx_lm.server, matching the startup message. - Added test_updates_mlxvlm.sh (mirrors test_updates_mlxlm.sh, asserting the transformers pin survives both the install and update paths) and test_quant_suffix.sh. Co-Authored-By: Claude Sonnet 5 --- demo/openCodeMlx | 57 ++++++++++++++++++++++++----- demo/test_quant_suffix.sh | 13 +++++++ demo/test_updates_mlxvlm.sh | 73 +++++++++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 9 deletions(-) create mode 100755 demo/test_quant_suffix.sh create mode 100755 demo/test_updates_mlxvlm.sh diff --git a/demo/openCodeMlx b/demo/openCodeMlx index e3ace42..a3781ae 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -154,15 +154,17 @@ ensure_mlx_lm_current() { # 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). Deliberately does not pin -# transformers here: mlx-lm's own pin above already constrains the shared -# venv, and guessing a separate mlx-vlm-specific range without evidence -# risks a silent version conflict between the two packages. +# 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; then + 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 @@ -174,7 +176,7 @@ ensure_mlx_vlm_current() { fi echo "Checking for mlx-vlm updates..." - if ! pip install --upgrade pip mlx-vlm; then + 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 @@ -280,6 +282,16 @@ _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"), 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. +_quant_suffix() { + local model_id="$1" + [[ "$model_id" =~ ([0-9]+bit)$ ]] && echo "${BASH_REMATCH[1]}" +} + # 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 @@ -495,12 +507,30 @@ main() { 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:-}" ]]; then + 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 '/' '--')" @@ -631,8 +661,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 @@ -648,7 +680,14 @@ EOF trap cleanup EXIT INT TERM HUP if [[ "$MAIN_MODEL_BACKEND" == "mlx_vlm" ]]; then - MAIN_CACHE_LIMIT="$MLX_CONTEXT_LIMIT" + # 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 diff --git a/demo/test_quant_suffix.sh b/demo/test_quant_suffix.sh new file mode 100755 index 0000000..0a18799 --- /dev/null +++ b/demo/test_quant_suffix.sh @@ -0,0 +1,13 @@ +#!/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')" + +report diff --git a/demo/test_updates_mlxvlm.sh b/demo/test_updates_mlxvlm.sh new file mode 100755 index 0000000..d13cfcd --- /dev/null +++ b/demo/test_updates_mlxvlm.sh @@ -0,0 +1,73 @@ +#!/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" +# no python3 stub -> `python3 -m mlx_vlm.server --help` fails against the real system +# interpreter (which has no mlx_vlm module), simulating "not installed" +( + 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" +( + 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 From 4d408bb3184ee704a8ee2981d866d60d2e0847c4 Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:19:21 +0200 Subject: [PATCH 07/18] feat(repl): wire /model, /context, /version, /stage, /revert, /commit-suggest, /applyBlocks These seven commands already existed as working ShellCommands methods (and were listed in help() and assistant.intent.allowed-commands) but had no case in CommandExecutor's dispatch switch and were absent from KNOWN_COMMANDS. That's the same structural gap /benchmark hit earlier: typing one of them verbatim fell through to "Unknown command" instead of either dispatching directly or reaching the LLM intent classifier for a command it doesn't describe either - closing part of the coverage risk flagged in the /benchmark misrouting review (a case added to the switch without a KNOWN_COMMANDS entry, or vice versa, silently breaks dispatch). While wiring --confirm/--dry-run/--secret-scan flags for /stage, /revert, /commit-suggest and /applyBlocks, hit the exact truthiness trap already fixed once for /benchmark's --max-tokens 0: "parseBoolean(x) ?: true" silently turns an explicit "--flag false" back into true, since Groovy's Elvis operator treats a parsed `false` as absent. Added a shared parseBooleanFlag(value, default) helper using an explicit null check instead, and used it everywhere a flag's default is `true` (so an explicit false actually takes effect) in this newly-wired code. Also fixed a stale test: `isKnownCommand('/model --set foo')` was asserting false from before /model was wired in. Co-Authored-By: Claude Sonnet 5 --- .../se/alipsa/lca/repl/CommandExecutor.groovy | 110 ++++++++++++++- .../lca/repl/CommandExecutorSpec.groovy | 127 +++++++++++++++++- 2 files changed, 234 insertions(+), 3 deletions(-) diff --git a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy index a2a1353..69d0aab 100644 --- a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy +++ b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy @@ -36,7 +36,8 @@ class CommandExecutor { 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" + "mcp", "reviewlog", "compact", "help", "health", "benchmark", "exit", "quit", + "model", "context", "version", "stage", "revert", "commit-suggest", "applyblocks" ) private final ShellCommands shellCommands @@ -113,6 +114,20 @@ class CommandExecutor { 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 @@ -385,6 +400,78 @@ class CommandExecutor { ) } + 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) + shellCommands.context( + filePath, + parseInt(parsed.start), + parseInt(parsed.end), + parsed.symbol as String, + parseInt(parsed.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") @@ -482,6 +569,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. */ @@ -511,6 +607,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/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy index 7839b75..7e76cf7 100644 --- a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy @@ -84,6 +84,128 @@ class CommandExecutorSpec extends Specification { 1 * shellCommands.gitPush(false, true) >> "pushed" } + 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 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: @@ -103,7 +225,8 @@ class CommandExecutorSpec extends Specification { 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" + "mcp", "reviewlog", "compact", "help", "health", "benchmark", + "model", "context", "version", "stage", "revert", "commit-suggest", "applyblocks" ] } @@ -121,7 +244,7 @@ class CommandExecutorSpec extends Specification { "/gitapply --patch x" || true "/git-apply --patch x" || true "/git-push --force" || true - "/model --set foo" || false + "/model --set foo" || true "/frobnicate" || false "review this please" || false null || false From dee6966d95c2b1f801b1b06bc407521d042facba Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:29:21 +0200 Subject: [PATCH 08/18] fix(openCodeMlx): disable speculative decoding by default, net perf loss Benchmarked via mlx_vlm.generate --verbose against MLX_MODEL: the Qwen3.8-27B-MTP-4bit drafter only achieved ~55% draft-token acceptance, making it a net loss - 12.5 tok/s generation with the drafter enabled vs 21.3 tok/s without it. MLX_DRAFT_MODEL now defaults to empty (speculative decoding off) instead of always attempting it. Also replaces LOCAL_MODEL_PROMPT with a looser variant (still grounds claims in shown content only, but drops the no-subagents/hard-limits rules that turned out to be too restrictive for this model). The prior working tree had two back-to-back assignments to the same variable - the first was dead (immediately shadowed by the second) and is removed here; only the surviving, actually-in-effect prompt remains. Co-Authored-By: Claude Sonnet 5 --- demo/openCodeMlx | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/demo/openCodeMlx b/demo/openCodeMlx index a3781ae..9f274fd 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -22,8 +22,12 @@ MLX_MODEL="${MLX_MODEL:-mlx-community/Qwen3.8-27B-4bit}" # 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. -MLX_DRAFT_MODEL="${MLX_DRAFT_MODEL:-mlx-community/Qwen3.8-27B-MTP-4bit}" +# 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 @@ -700,13 +704,13 @@ EOF exit 1 fi + # Looser variant of the original strict prompt: still grounds claims in shown content only, + # but drops the no-subagents/hard-limits rules that turned out to be too restrictive. LOCAL_MODEL_PROMPT="You are running on a local LLM with limited capacity. Follow these rules strictly: ## Ground in shown content only - Base every factual claim about code (a bug, a missing import, an undefined name, incorrect logic, a behavior change) only on file contents, diffs, or tool output actually shown in this conversation. - Do not draw on anything you recall about this repository, a similarly named project, or a similar package from training data - even if the name looks familiar, assume you have not seen its real current contents unless it was shown here in this conversation. - Before asserting that an issue exists, quote the exact line(s) from the tool output or file read that support the claim. If you cannot quote a specific line backing the claim, do not make the claim - say you could not confirm it instead. -## No subagents -Do NOT spawn sub-agents or delegate to other agents. Work directly in the main conversation using read, edit, write, and bash tools. Subagents will loop and waste time on this model. ## Small steps only When asked to implement a plan or feature: 1. Read the plan file first @@ -714,15 +718,11 @@ When asked to implement a plan or feature: 3. Work through them one at a time 4. Each step should be: read the relevant file, make the change, verify it compiles/passes ## Be concrete -- Never explore broadly. Ask the user which files to look at if unsure. - Never churn. If you do not know what to do next, ask the user immediately. -- Never spend more than 5 tool calls exploring before starting to write code. -## Keep responses extremely short -- NEVER exceed 2000 tokens in a single response. If more work is needed, use multiple tool calls instead of writing long text. -- Maximum 2 sentences of explanation between tool calls +## Keep responses short +- Use multiple tool calls instead of writing long text. - No summaries, no restating the plan, no listing what you will do - Do NOT output the entire file content when editing - use the edit tool with only the changed section -- Do NOT think out loud or explain your reasoning. Just act. ## When stuck If you find yourself making more than 8 tool calls without producing a code change, STOP and tell the user what is blocking you. Do not keep searching." From d77709cabd9792a376dec2a375d54b9a215e32fc Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:35:22 +0200 Subject: [PATCH 09/18] fix(openCodeMlx): _quant_suffix no longer aborts main() under set -e main() runs under set -e, and its call sites assign _quant_suffix's result via bare "main_quant=$(_quant_suffix "$MLX_MODEL")" - not inside an if/while condition, so the assignment takes the exit status of the command substitution directly. _quant_suffix's body was "[[ ... ]] && echo ...", which returns 1 (the [[ ]]'s own status) on a no-match model id, silently killing the whole script right after the multi-GB model download - no error message, and no cleanup trap installed yet (that happens later in main()). Trigger: mlx_vlm backend + MLX_DRAFT_MODEL set (the default) + an MLX_MODEL id not ending in "bit" (e.g. an unquantized/bf16 VLM checkpoint). _quant_suffix now always returns 0, echoing the match (or nothing) without letting a non-match propagate as a command failure. Added a set -e regression test to test_quant_suffix.sh (asserts a bare assignment under set -e still reaches the next line on a non-matching model id) - the existing tests only checked stdout via $(...), which never runs under set -e and so never exercised this path. Co-Authored-By: Claude Sonnet 5 --- demo/openCodeMlx | 9 ++++++++- demo/test_quant_suffix.sh | 12 ++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/demo/openCodeMlx b/demo/openCodeMlx index 9f274fd..db4b2e6 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -293,7 +293,14 @@ _download_via() { # silently serving a mismatched drafter. _quant_suffix() { local model_id="$1" - [[ "$model_id" =~ ([0-9]+bit)$ ]] && echo "${BASH_REMATCH[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)$ ]]; then + echo "${BASH_REMATCH[1]}" + fi + return 0 } # detect_model_backend : echoes "mlx_vlm" if the downloaded diff --git a/demo/test_quant_suffix.sh b/demo/test_quant_suffix.sh index 0a18799..6472b69 100755 --- a/demo/test_quant_suffix.sh +++ b/demo/test_quant_suffix.sh @@ -10,4 +10,16 @@ check "extracts a multi-digit bit suffix" "16bit" "$(_quant_suffix 'mlx-communit 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')" +# 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 From b5a42a6199eb5e1a7ca509abd5b74452daa3dd6c Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:35:34 +0200 Subject: [PATCH 10/18] fix(repl): /git-push --confirm false was silently forced back to true parseBoolean(parsed.confirm) ?: true hits the same Groovy truthiness trap parseBooleanFlag was introduced to fix two commits ago: parseBoolean returns Boolean.FALSE for an explicit "--confirm false", and Elvis treats that false as absent, re-enabling the confirmation prompt the caller asked to skip. Switched to parseBooleanFlag(parsed.confirm, true). Found and fixed the identical pre-existing bug in executeGitApply (--check, --confirm) and executeApply (--dry-run, --confirm) while looking - same class of defect, same fix already available. Added dispatch/regression tests for /git-push, /gitapply, and /apply covering both the happy path and an explicit "false" override of each previously-broken flag. Co-Authored-By: Claude Sonnet 5 --- .../se/alipsa/lca/repl/CommandExecutor.groovy | 10 ++--- .../lca/repl/CommandExecutorSpec.groovy | 45 +++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy index 69d0aab..28f4268 100644 --- a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy +++ b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy @@ -301,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) ) } @@ -313,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) ) } @@ -382,7 +382,7 @@ class CommandExecutor { Map parsed = parseArgs(args) shellCommands.gitPush( parseBoolean(parsed.force) ?: false, - parseBoolean(parsed.confirm) ?: true + parseBooleanFlag(parsed.confirm, true) ) } diff --git a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy index 7e76cf7..ec66594 100644 --- a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy @@ -84,6 +84,51 @@ class CommandExecutorSpec extends Specification { 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') From 3bf42968a45f393fb0c1799c5305bcdbd204333b Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:52:36 +0200 Subject: [PATCH 11/18] fix(openCodeMlx): _quant_suffix now matches DWQ-style quant suffixes ([0-9]+bit)$ didn't match ids like "...-4bit-DWQ" or "...-8bit_dwq", so the draft/main quant-mismatch guard silently no-op'd for those - a mismatched drafter could still be served without the fail-fast check ever firing. Now allows exactly one optional "-word"/"_word" qualifier after the bit count. Deliberately NOT "([0-9]+bit)([-_][A-Za-z]+)*$" (zero-or-more): tested that against the existing "does not match a mid-string bit token" case and it wrongly matches "8bit-prefixed-model" as an 8bit id, since a multi-word non-quant tail ("-prefixed-model") also satisfies a repeated-group pattern. The tightened single-optional-group version passes both the DWQ cases and that existing negative case. Co-Authored-By: Claude Sonnet 5 --- demo/openCodeMlx | 15 +++++++++------ demo/test_quant_suffix.sh | 3 +++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/demo/openCodeMlx b/demo/openCodeMlx index db4b2e6..0c33ef5 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -286,18 +286,21 @@ _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"), 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. +# _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)$ ]]; then + if [[ "$model_id" =~ ([0-9]+bit)([-_][A-Za-z]+)?$ ]]; then echo "${BASH_REMATCH[1]}" fi return 0 diff --git a/demo/test_quant_suffix.sh b/demo/test_quant_suffix.sh index 6472b69..2461b64 100755 --- a/demo/test_quant_suffix.sh +++ b/demo/test_quant_suffix.sh @@ -9,6 +9,9 @@ check "extracts an 8bit suffix" "8bit" "$(_quant_suffix 'mlx-community/Qwen3.8-2 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 From 0800ed2cb7a81160c434008d37b2cb6de0e12343 Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:52:49 +0200 Subject: [PATCH 12/18] revert(openCodeMlx): un-loosen LOCAL_MODEL_PROMPT, restore strict rules Reverts the prompt half of dee6966 (no-subagents, exploration-budget, and response-length rules removed there). That change was bundled into an otherwise evidence-backed drafter-perf commit with no evidence of its own beyond "turned out to be too restrictive," and it landed on exactly the model (4-bit) already flagged in this project's own history as unable to follow even the surviving grounding rules regardless of prompt phrasing - not the model to be loosening guardrails on without data showing it helps. The drafter/perf fix from that commit is unaffected. Co-Authored-By: Claude Sonnet 5 --- demo/openCodeMlx | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/demo/openCodeMlx b/demo/openCodeMlx index 0c33ef5..d77a0ae 100755 --- a/demo/openCodeMlx +++ b/demo/openCodeMlx @@ -714,13 +714,13 @@ EOF exit 1 fi - # Looser variant of the original strict prompt: still grounds claims in shown content only, - # but drops the no-subagents/hard-limits rules that turned out to be too restrictive. LOCAL_MODEL_PROMPT="You are running on a local LLM with limited capacity. Follow these rules strictly: ## Ground in shown content only - Base every factual claim about code (a bug, a missing import, an undefined name, incorrect logic, a behavior change) only on file contents, diffs, or tool output actually shown in this conversation. - Do not draw on anything you recall about this repository, a similarly named project, or a similar package from training data - even if the name looks familiar, assume you have not seen its real current contents unless it was shown here in this conversation. - Before asserting that an issue exists, quote the exact line(s) from the tool output or file read that support the claim. If you cannot quote a specific line backing the claim, do not make the claim - say you could not confirm it instead. +## No subagents +Do NOT spawn sub-agents or delegate to other agents. Work directly in the main conversation using read, edit, write, and bash tools. Subagents will loop and waste time on this model. ## Small steps only When asked to implement a plan or feature: 1. Read the plan file first @@ -728,11 +728,15 @@ When asked to implement a plan or feature: 3. Work through them one at a time 4. Each step should be: read the relevant file, make the change, verify it compiles/passes ## Be concrete +- Never explore broadly. Ask the user which files to look at if unsure. - Never churn. If you do not know what to do next, ask the user immediately. -## Keep responses short -- Use multiple tool calls instead of writing long text. +- Never spend more than 5 tool calls exploring before starting to write code. +## Keep responses extremely short +- NEVER exceed 2000 tokens in a single response. If more work is needed, use multiple tool calls instead of writing long text. +- Maximum 2 sentences of explanation between tool calls - No summaries, no restating the plan, no listing what you will do - Do NOT output the entire file content when editing - use the edit tool with only the changed section +- Do NOT think out loud or explain your reasoning. Just act. ## When stuck If you find yourself making more than 8 tool calls without producing a code change, STOP and tell the user what is blocking you. Do not keep searching." From 14bdd5419ab6af7963ce234d88863fd469d05efd Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:53:02 +0200 Subject: [PATCH 13/18] fix(repl): last three Elvis-boolean/int truthiness sites in CommandExecutor Same trap fixed in the last two commits, now closed everywhere it was still reachable from newly-touched or newly-wired code: - /review --log-review false was silently forced back to true (parseBoolean(x) ?: true). - /search --headless false was silently forced back to true, same pattern. - /run --confirm false was silently forced back to true - the same user-visible defect just fixed for /apply, /gitapply and /git-push. - /context --padding 0 fell back to the default 2: ShellCommands.context explicitly allows 0 (requireMin(padding, 0, ...)), but "parseInt(x) ?: 2" treats a parsed 0 as absent, the int-flavored version of the same bug already fixed for /benchmark's --max-tokens 0. All four switched to parseBooleanFlag / an explicit null check, with a dispatch test asserting the explicit "false"/"0" case for each. Co-Authored-By: Claude Sonnet 5 --- .../se/alipsa/lca/repl/CommandExecutor.groovy | 12 +++-- .../lca/repl/CommandExecutorSpec.groovy | 44 +++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy index 28f4268..e255e0e 100644 --- a/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy +++ b/src/main/groovy/se/alipsa/lca/repl/CommandExecutor.groovy @@ -230,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, @@ -247,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 ) } @@ -260,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 ) } @@ -412,12 +412,16 @@ class CommandExecutor { 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, - parseInt(parsed.padding) ?: 2 + padding != null ? padding : 2 ) } diff --git a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy index ec66594..923ebea 100644 --- a/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/repl/CommandExecutorSpec.groovy @@ -35,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"' @@ -47,6 +56,30 @@ 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' @@ -163,6 +196,17 @@ class CommandExecutorSpec extends Specification { 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') From 3b7a5fe7d38932bd3cc5b01d664b70df0950fcef Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 16:53:12 +0200 Subject: [PATCH 14/18] test(scripts): run demo/test_*.sh as part of ./mvnw test test_quant_suffix.sh existed and would have caught the set -e/exit-1 regression fixed a few commits back, but nothing ran it as part of the build - it only ran when someone remembered to invoke it by hand, which is exactly how that regression shipped unnoticed. Discovers every demo/test_*.sh (except the shared test_helpers.sh library) at test time and asserts each exits 0 with no "FAIL:" lines, rather than hardcoding a list that would itself go stale the next time a demo test file is added. Co-Authored-By: Claude Sonnet 5 --- .../lca/scripts/DemoTestScriptsSpec.groovy | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy 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..6439beb --- /dev/null +++ b/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy @@ -0,0 +1,52 @@ +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.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 { + + @Unroll + def "demo test script passes: #scriptName"() { + given: + Path scriptPath = demoDir().resolve(scriptName) + + when: + ProcessBuilder processBuilder = new ProcessBuilder("bash", scriptPath.toString()) + processBuilder.directory(demoDir().toFile()) + processBuilder.redirectErrorStream(true) + Process process = processBuilder.start() + String output = process.inputStream.text + int exitCode = process.waitFor() + + then: + exitCode == 0 + !output.contains("FAIL:") + + where: + scriptName << demoTestScriptNames() + } + + private static Path demoDir() { + Paths.get("").toAbsolutePath().normalize().resolve("demo") + } + + private static List demoTestScriptNames() { + Files.list(demoDir()).withCloseable { stream -> + stream.map { it.fileName.toString() } + .filter { it.startsWith("test_") && it.endsWith(".sh") && it != "test_helpers.sh" } + .sorted() + .collect(Collectors.toList()) + } + } +} From 3fccae33f94d6ae81ab228b3f545c3c140557a5c Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 17:07:21 +0200 Subject: [PATCH 15/18] fix(demo): stop three not-installed test scenarios from inheriting ambient state test_updates_mlxvlm.sh, test_updates_mlxlm.sh and test_updates_opencode.sh's "not installed" scenarios all relied on a tool's absence from a fixed system PATH being a property of the CI/dev machine, not of the test. Verified by reproducing the exact flip: stubbing python3 to succeed made test_updates_mlxvlm.sh's scenario B assert 1, get 0 - the "already installed -> warn and continue" branch fired instead of the intended "not installed -> hard error" branch. This was harmless while nobody ran these scripts as part of a build; wiring them into ./mvnw test (DemoTestScriptsSpec) made a real machine's local Python/tooling state load-bearing for whether the build passes. - test_updates_mlxlm.sh / test_updates_mlxvlm.sh: explicitly stub mlx_lm.server / python3 to fail in the "not installed" scenarios, instead of relying on the real system binary being absent. - test_updates_opencode.sh: can't stub "opencode is absent" the same way (opencode itself is the thing being checked, not something routed through a shared interpreter/pip). Added a guard that checks whether a real opencode is resolvable on the test's REAL_PATH before running the "not installed" scenarios (A, B, E) and skips them with a clear message if so, rather than letting them silently exercise the "already installed -> real opencode upgrade" branch - a real network call against whatever opencode install actually exists on that machine. Co-Authored-By: Claude Sonnet 5 --- demo/test_updates_mlxlm.sh | 8 +++- demo/test_updates_mlxvlm.sh | 10 ++++- demo/test_updates_opencode.sh | 70 +++++++++++++++++++++++------------ 3 files changed, 62 insertions(+), 26 deletions(-) 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 index d13cfcd..bf0effb 100755 --- a/demo/test_updates_mlxvlm.sh +++ b/demo/test_updates_mlxvlm.sh @@ -19,8 +19,13 @@ 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 python3 stub -> `python3 -m mlx_vlm.server --help` fails against the real system -# interpreter (which has no mlx_vlm module), simulating "not installed" +# 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 ) @@ -31,6 +36,7 @@ check "not-installed -> pip carries the shared transformers pin" "1" "$(grep -c 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 ) diff --git a/demo/test_updates_opencode.sh b/demo/test_updates_opencode.sh index 978e52c..b218f0e 100755 --- a/demo/test_updates_opencode.sh +++ b/demo/test_updates_opencode.sh @@ -48,30 +48,50 @@ 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 below all assume opencode is NOT resolvable on REAL_PATH, so that +# ensure_opencode_current takes its "not found -> install" branch. That assumption is a +# property of the machine running this test, not of the code under test: on a machine +# where a real `opencode` binary lives in /usr/bin, /bin, or /usr/local/bin, these +# scenarios would silently exercise the "already installed -> opencode upgrade" branch +# instead - running a REAL network-touching "opencode upgrade" against the machine's own +# install, rather than the intended install-simulation path. Guard explicitly instead of +# inheriting the absence: skip (not fail) when the precondition doesn't hold, since this +# is an environment property, not a code defect. +if PATH="$REAL_PATH" command -v opencode >/dev/null 2>&1; then + echo "SKIP: a real 'opencode' is resolvable on REAL_PATH ($REAL_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="$REAL_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="$REAL_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 +128,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 REAL_PATH-must-lack-opencode +# assumption as scenarios A/B (test_set_e_with_failed_install hardcodes it 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 From 3b1d672f2ff6027ec0643828c263aef9fba6d746 Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 17:07:38 +0200 Subject: [PATCH 16/18] fix(scripts): bound DemoTestScriptsSpec's process wait, pin discovery to tracked files process.waitFor() had no timeout, and since one of these scripts (test_updates_opencode.sh) can reach a real, network-touching "opencode upgrade" on a machine where opencode is on PATH, an unbounded wait turns that into a hung ./mvnw test instead of a failure. Note that process.inputStream.text (the prior way of collecting output) itself blocks until the stream closes - a timeout on waitFor() alone wouldn't have helped, since a hung process would never reach that call. Output is now collected on a background thread concurrently with a bounded process.waitFor(120s), with destroyForcibly() on expiry. Also pins script discovery to `git ls-files demo` output, so an untracked scratch script (e.g. a developer's local demo/test_whatever.sh) can't silently join the build - falls back to unfiltered disk listing only if git itself is unavailable. Verified by dropping an untracked test_scratch_untracked.sh (that would fail loudly if run) into demo/ and confirming it's excluded from the discovered script list. Co-Authored-By: Claude Sonnet 5 --- .../lca/scripts/DemoTestScriptsSpec.groovy | 60 ++++++++++++++++++- 1 file changed, 57 insertions(+), 3 deletions(-) diff --git a/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy b/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy index 6439beb..2222ff0 100644 --- a/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy @@ -6,6 +6,7 @@ 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 /** @@ -16,6 +17,8 @@ import java.util.stream.Collectors */ class DemoTestScriptsSpec extends Specification { + private static final long TIMEOUT_SECONDS = 120L + @Unroll def "demo test script passes: #scriptName"() { given: @@ -26,11 +29,29 @@ class DemoTestScriptsSpec extends Specification { processBuilder.directory(demoDir().toFile()) processBuilder.redirectErrorStream(true) Process process = processBuilder.start() - String output = process.inputStream.text - int exitCode = process.waitFor() + // Read stdout on a separate thread: some of these scripts resolve tools (opencode, + // python3, pip) off the ambient PATH and can reach a real, network-touching command + // on a misconfigured environment. process.inputStream.text blocks until the stream + // closes, so a hung subprocess would hang here even before any waitFor() timeout is + // reached - the reader has to run concurrently with the bounded wait below, not before it. + StringBuilder outputBuffer = new StringBuilder() + Thread reader = new Thread({ -> + process.inputStream.eachLine { line -> outputBuffer.append(line).append('\n') } + } as Runnable) + reader.daemon = true + reader.start() + + boolean finished = process.waitFor(TIMEOUT_SECONDS, TimeUnit.SECONDS) + if (!finished) { + process.destroyForcibly() + process.waitFor(5, TimeUnit.SECONDS) + } + reader.join(5000) + String output = outputBuffer.toString() then: - exitCode == 0 + finished + process.exitValue() == 0 !output.contains("FAIL:") where: @@ -42,11 +63,44 @@ class DemoTestScriptsSpec extends Specification { } private static List demoTestScriptNames() { + List tracked = gitTrackedDemoTestScripts() 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()) } } + + /** + * 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" (empty exclusion) only if git itself is unavailable, since a + * missing git binary 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) + Process process = processBuilder.start() + String output = process.inputStream.text + boolean finished = process.waitFor(30, TimeUnit.SECONDS) + if (!finished) { + process.destroyForcibly() + return demoTestScriptNamesOnDisk() + } + if (process.exitValue() != 0) { + return demoTestScriptNamesOnDisk() + } + 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()) + } + } } From 328d54f494deb255bed211f482f8d2868e5a569c Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 18:28:30 +0200 Subject: [PATCH 17/18] fix(scripts): DemoTestScriptsSpec's own git subprocess had the bug it just fixed gitTrackedDemoTestScripts()'s `git ls-files` call did "String output = process.inputStream.text" (a blocking read to EOF) followed by a bounded waitFor(30s) - the exact blocking-read-before- bounded-wait bug the main test-runner path had just been fixed for in the same commit. A hung git (index lock, credential prompt) would hang on the read before the timeout was ever reached, making it decorative. Extracted the concurrent-reader pattern into a shared runBounded() helper used by both the demo-script runner and the git subprocess, so there's one place this gets fixed instead of two copies to keep in sync. Also: - Switched the shared output buffer from StringBuilder to StringBuffer. reader.join(5000) only supplies a happens-before edge when the reader thread actually terminates in time; on the timeout/force-kill path it may not have, leaving outputBuffer.toString() as an unsynchronized read of a StringBuilder that could still be mid-append on another thread. StringBuffer's synchronized methods close that gap. - Added an explicit assertion when the git-tracked-name filter narrows discovery to zero scripts, with a message pointing at the tracked-files lookup specifically (not "demo/ has no tests"). Verified first that Spock actually already throws "Data provider has no data" on an empty where: list rather than silently running zero iterations - so the silent-green-build failure mode itself doesn't occur, but the assertion gives a far clearer diagnosis than that generic message would. Co-Authored-By: Claude Sonnet 5 --- .../lca/scripts/DemoTestScriptsSpec.groovy | 99 ++++++++++++------- 1 file changed, 63 insertions(+), 36 deletions(-) diff --git a/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy b/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy index 2222ff0..52b1afd 100644 --- a/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy +++ b/src/test/groovy/se/alipsa/lca/scripts/DemoTestScriptsSpec.groovy @@ -18,41 +18,23 @@ import java.util.stream.Collectors 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) - - when: ProcessBuilder processBuilder = new ProcessBuilder("bash", scriptPath.toString()) processBuilder.directory(demoDir().toFile()) processBuilder.redirectErrorStream(true) - Process process = processBuilder.start() - // Read stdout on a separate thread: some of these scripts resolve tools (opencode, - // python3, pip) off the ambient PATH and can reach a real, network-touching command - // on a misconfigured environment. process.inputStream.text blocks until the stream - // closes, so a hung subprocess would hang here even before any waitFor() timeout is - // reached - the reader has to run concurrently with the bounded wait below, not before it. - StringBuilder outputBuffer = new StringBuilder() - Thread reader = new Thread({ -> - process.inputStream.eachLine { line -> outputBuffer.append(line).append('\n') } - } as Runnable) - reader.daemon = true - reader.start() - boolean finished = process.waitFor(TIMEOUT_SECONDS, TimeUnit.SECONDS) - if (!finished) { - process.destroyForcibly() - process.waitFor(5, TimeUnit.SECONDS) - } - reader.join(5000) - String output = outputBuffer.toString() + when: + ProcessResult result = runBounded(processBuilder, TIMEOUT_SECONDS) then: - finished - process.exitValue() == 0 - !output.contains("FAIL:") + result.finished + result.exitCode == 0 + !result.output.contains("FAIL:") where: scriptName << demoTestScriptNames() @@ -64,36 +46,39 @@ class DemoTestScriptsSpec extends Specification { private static List demoTestScriptNames() { List tracked = gitTrackedDemoTestScripts() - Files.list(demoDir()).withCloseable { stream -> + 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" (empty exclusion) only if git itself is unavailable, since a - * missing git binary is an environment problem this spec shouldn't mask a script list over. + * 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) - Process process = processBuilder.start() - String output = process.inputStream.text - boolean finished = process.waitFor(30, TimeUnit.SECONDS) - if (!finished) { - process.destroyForcibly() - return demoTestScriptNamesOnDisk() - } - if (process.exitValue() != 0) { + ProcessResult result = runBounded(processBuilder, GIT_TIMEOUT_SECONDS) + if (!result.finished || result.exitCode != 0) { return demoTestScriptNamesOnDisk() } - output.readLines() + result.output.readLines() .findAll { it.startsWith("demo/") } .collect { it.substring("demo/".length()) } } @@ -103,4 +88,46 @@ class DemoTestScriptsSpec extends Specification { 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()) + } } From 90ced21c5e73abc04113276d4026610b2a91a567 Mon Sep 17 00:00:00 2001 From: pernyf Date: Fri, 28 Aug 2026 18:28:42 +0200 Subject: [PATCH 18/18] fix(demo): make opencode's not-installed test scenarios skip only in the rarest case The previous fix made scenarios A/B/E skip (rather than silently mis-exercise) when a real opencode is resolvable on REAL_PATH - but DemoTestScriptsSpec only asserts exit 0 and no "FAIL:", and a skip satisfies both, so on a machine with opencode on REAL_PATH those three scenarios would quietly disappear from coverage while the build still reported green. Narrowed the PATH used by scenarios A/B/E (and the guard's own check) from REAL_PATH ("/usr/bin:/bin:/usr/local/bin") to "/usr/bin:/bin" - dropping the one directory (/usr/local/bin) a system-wide opencode install might realistically symlink into on this project's target platform. The official installer already puts opencode in ~/.opencode/bin, off REAL_PATH to begin with; /usr/bin and /bin alone still cover every coreutil (mkdir/cat/chmod/bash/sh) these scenarios use, verified by confirming each resolves under /bin on this machine. This converts the skip from "silently hit on common developer/Homebrew setups" to "only hit if opencode is symlinked directly into /usr/bin or /bin" - essentially never, while keeping the guard itself as a defense-in-depth fallback rather than removing it. Co-Authored-By: Claude Sonnet 5 --- demo/test_updates_opencode.sh | 39 ++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/demo/test_updates_opencode.sh b/demo/test_updates_opencode.sh index b218f0e..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,17 +48,22 @@ trap 'rm -rf "$work"' EXIT # Step 3), which tests override directly as a bash function instead. REAL_PATH="/usr/bin:/bin:/usr/local/bin" -# Scenarios A/B/E below all assume opencode is NOT resolvable on REAL_PATH, so that -# ensure_opencode_current takes its "not found -> install" branch. That assumption is a -# property of the machine running this test, not of the code under test: on a machine -# where a real `opencode` binary lives in /usr/bin, /bin, or /usr/local/bin, these -# scenarios would silently exercise the "already installed -> opencode upgrade" branch -# instead - running a REAL network-touching "opencode upgrade" against the machine's own -# install, rather than the intended install-simulation path. Guard explicitly instead of -# inheriting the absence: skip (not fail) when the precondition doesn't hold, since this -# is an environment property, not a code defect. -if PATH="$REAL_PATH" command -v opencode >/dev/null 2>&1; then - echo "SKIP: a real 'opencode' is resolvable on REAL_PATH ($REAL_PATH) in this" >&2 +# 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 @@ -79,7 +84,7 @@ INNER chmod +x "$home_a/.opencode/bin/opencode" } ( - PATH="$REAL_PATH" HOME="$home_a" ensure_opencode_current >/dev/null 2>&1 + PATH="$NOT_INSTALLED_PATH" HOME="$home_a" ensure_opencode_current >/dev/null 2>&1 ) check "not-installed + install produces a binary -> returns 0" "0" "$?" @@ -88,7 +93,7 @@ INNER home_b="$work/b_home" _install_opencode_via_curl() { :; } ( - PATH="$REAL_PATH" HOME="$home_b" ensure_opencode_current >/dev/null 2>&1 + 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 @@ -128,9 +133,9 @@ 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). Same REAL_PATH-must-lack-opencode -# assumption as scenarios A/B (test_set_e_with_failed_install hardcodes it internally) - -# guarded above, so only run this when that guard held. +# 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" "$?"