scout-cli: qwen3:8b harness default, cap Ollama generations, Windows-safe herd kill - #68
Merged
Merged
Conversation
llm_exec.DEFAULT_OLLAMA_MODEL was qwen2.5:7b-instruct, which is not pulled on this box (`ollama list`: only qwen3:8b and agentos-qwen). Without DOTTIE_LLM_MODEL set, the llm tier's "reachable" check still passed (Ollama is running) but `ollama pull qwen2.5:7b-instruct` was never actually run here, so a bare run of this tier failed at completion time -- the harness's own ROUTER_LABELS_RUNBOOK.md example never worked on this box either. Cam's call (2026-09-27, PR #67 review): move the default to qwen3:8b even though it changes which model produces "scout router probe" labels -- a probe run without DOTTIE_LLM_MODEL set now records backend `ollama:qwen3:8b` instead of `ollama:qwen2.5:7b-instruct`. Updates docs/ROUTER_LABELS_RUNBOOK.md and docs/ARCHITECTURE.md, which both named the old default. Test expectations in test_harness_executors.py move to the new default (the router-label change above, not a loosened assertion), plus a pin test so the default can't silently drift again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LHC4Z5q8HGJxLCSG3h8Db
None of scout-cli's five direct /api/chat|/api/generate callers sent a generation cap: bigbang.core.llm.chat_with_metrics ignored its own max_tokens argument on the ollama branch (it already honored it for koboldcpp), and ollama_chat / _ollama_generate sent no options at all. The fallback direct posts in agent/cli.py, ava/cli.py and write/cli.py (grepped for `/api/chat` and `/api/generate`) had the same gap, and so did `scout ollama run`'s own --num-predict, which sent nothing when the flag was omitted. apps/dottie hit the real failure mode this closes (PR #66): with llama-server's --context-shift, an uncapped call does not error, it just keeps generating — one run went 708s to 81,920 tokens. Adds bigbang.core.llm.resolve_num_predict(): explicit argument > DOTTIE_OLLAMA_NUM_PREDICT env > 2048 default; 0 or negative means no cap. This is the SAME env var apps/dottie's OllamaPolicy reads (PR #66), so one setting caps every local Ollama caller on the box, and a malformed value raises rather than silently running uncapped (mirrored from that same PR). Wired into chat_with_metrics/ollama_chat/_ollama_generate directly; the three duplicate fallback code paths (agent/cli.py, ava/cli.py: only reachable when importing bigbang.core.llm itself has already failed; write/cli.py: its own primary /api/chat call) each get their own small env-read rather than an import from a module that, in the agent/ava cases, is by definition unusable there. `scout ollama run`'s --num-predict now resolves the same way instead of sending no cap on an omitted flag; bigbang/core/ollama.py's complete() is untouched — it is documented as pure options passthrough and has its own test asserting an omitted `options=` sends none, which is correct for that layer. scripts/bench_local_runner.py already passes an explicit --max-tokens through chat_with_metrics on both backends; this fixes an existing asymmetry where koboldcpp respected it and ollama silently did not, so a prior ollama-vs-kobold bench comparison ran ollama uncapped against a capped kobold run. Coverage: resolve_num_predict's precedence/parsing, chat_with_metrics's default/explicit/malformed-env cases (including that a malformed cap fails closed rather than running uncapped), ollama_chat, scout-cli's own _options()/--num-predict CLI path (default, override, zero-means-no-cap, malformed-env-fails-actionably), and write/cli.py's _ollama_chat. The agent/cli.py and ava/cli.py fallback branches are fixed but not exercised by an automated test — they only run when importing bigbang.core.llm has already failed, which does not happen in this repo today, and forcing that import failure reliably under pytest is disproportionate to a defensive fallback. Documents DOTTIE_OLLAMA_NUM_PREDICT in docs/ROUTER_LABELS_RUNBOOK.md (added in the prior commit alongside the qwen3:8b default line, ahead of this one landing it — a doc-only forward reference, not a functional dependency). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LHC4Z5q8HGJxLCSG3h8Db
close_session's kill path called os.killpg(pid, signal.SIGTERM) unconditionally on every platform, catching only ProcessLookupError. Neither os.killpg nor signal.SIGKILL exist on Windows at all (os.killpg is not a module attribute there; signal.SIGKILL is not defined), so the first line raised AttributeError, uncaught. This was latent until PR #67 made _pid_alive correct on Windows -- before that, alive() could not be trusted enough to route here in the first place. Now it can, so the crash is real: `herd close --kill` against a live session cannot complete on Windows at all. Extracts the existing TERM-then-KILL sequence into _kill_session_posix() unchanged (same exception handling, same 0.2s grace sleep), adds _kill_session_win32() using `taskkill /PID <pid> /T /F` via subprocess with an argument list (never shell=True -- scripts/check_shell_true.py gates that), and dispatches on sys.platform in close_session, the same pattern _pid_alive already uses. /T is load-bearing: the pid on record is start_session's supervisor, not the real command, so a plain `taskkill /PID` would leave the actual work running. /F is the closest single-shot equivalent to SIGKILL -- an ordinary Windows console process has no SIGTERM analogue for an external caller, so there is no honest graceful-then-forceful staging to add on that side; one forceful pass is correct, not a shortcut. Tests: both branches run on whichever platform executes the suite (POSIX tests fake os.killpg/os.kill/signal.SIGKILL with raising=False, since none of the three exist on this Windows dev box either -- the same reasoning TestPidAlive already documents for why CI being ubuntu-only isn't enough), a dispatcher test per platform via sys.platform, and one real end-to-end test: start a real long-running child through the CLI, `herd close --kill --force` it, and confirm the pid is actually gone (not just removed from the ledger). That end-to-end test is a genuine regression check on this box -- it fails against the pre-fix code with the exact AttributeError this commit removes. herd/cli.py's --kill help text and a start_session comment referencing "killpg's the new session group" are updated to name both platforms. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LHC4Z5q8HGJxLCSG3h8Db
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Three minor findings from PR #68's review, all confirmed: 1. ollama_chat()'s `cap = resolve_num_predict(num_predict)` sat inside its own broad `except Exception: return None`, so a malformed DOTTIE_OLLAMA_NUM_PREDICT degraded to the same generic None a down server or network error returns, instead of the actionable ValueError resolve_num_predict raises. Moved the resolve call before the client opens and before the try, so the raise reaches the caller. The review's evidence claimed _ollama_generate/chat_with_metrics already contrasted with this by propagating the ValueError into meta['error'] — empirically false (confirmed with a live repro before touching any code): _ollama_generate had the IDENTICAL swallow, so chat_with_metrics reported 'backend returned no completion' for a garbage cap too, not the env var's name. Fixed the same way there, and tightened the existing test_ollama_chat_with_metrics_a_garbage_cap_fails_closed_not_uncapped to assert the message now names DOTTIE_OLLAMA_NUM_PREDICT (it failed against pre-fix code, confirming it tests something real). Added test_ollama_chat_a_garbage_cap_raises_not_silently_none for ollama_chat() itself. Left ava/cli.py's _route_with_ollama untouched: it wraps _core_ollama_chat in its own pre-existing `except Exception: raw = None`, so `scout ava` still degrades to keyword routing on a bad env var rather than surfacing the message — a separate, pre-existing resilience design in that plugin, out of this finding's scope. 2. agent/cli.py's and ava/cli.py's fallback ollama_chat/_route_with_ollama duplicates (reachable only when importing bigbang.core.llm has already failed) shipped in PR #67/#68 with no automated coverage — confirmed via `bigbang.plugins.ava.cli._HAS_CORE_LLM` being True in this repo's real environment, so the branch never ran. Added one test per plugin that forces the import failure with `sys.modules["bigbang.core.llm"] = None` (the documented sys.modules sentinel for ImportError, not a monkeypatch of real internals), reloads the plugin so it takes the fallback branch, and drives the real function with a fake httpx client — restoring and reloading again in `finally` so no other test sees the fallback state. 3. bigbang/core/ollama.py's complete() has no default generation cap of its own — deliberate (it's documented as pure options passthrough with its own test), but nothing said so at the call site. Added a one-line comment. Verified: uv run pytest tests -q from apps/scout-cli — 2686 passed, 1 failed, 2 skipped (243s). The one failure is the pre-existing tests/test_loop_plugin.py::test_evaluate_blocked_on_stale_metrics_exits_2 (fcntl import in packages/dottie-loop, Windows-only, unrelated to this PR). 2686 = the PR's prior 2683 + these 3 new tests. check_documented_counts.py --check: 970 reproduces, unchanged. gate_audit.py --check: no new candidates. goat_audit.py --check --min-mean 7.0: no regressions vs baseline. check_declared_capabilities.py / check_resolver_fallbacks.py / check_shell_true.py / check_cli_path_args.py / store_symmetry_audit.py / dag_next.py / check_todos_timestamps.py / check_handoff_fresh.py: all clean. leaks scan .: clean. State-pollution sweep around the new/changed tests: CLEAN. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LHC4Z5q8HGJxLCSG3h8Db
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Summary
Three follow-ups from PR #67's review, each its own commit:
1.
e97a5cd— harness llm tier default → qwen3:8bllm_exec.DEFAULT_OLLAMA_MODELwasqwen2.5:7b-instruct, which is not pulled on this box (ollama list: onlyqwen3:8bandagentos-qwen). Cam's call (2026-09-27): move the default toqwen3:8b. This changes which model produces "scout router probe" labels — a probe run withoutDOTTIE_LLM_MODELset now records backendollama:qwen3:8binstead ofollama:qwen2.5:7b-instruct. Updatesdocs/ROUTER_LABELS_RUNBOOK.mdanddocs/ARCHITECTURE.md, which both named the old default.2.
3989b16— cap every Ollama generationNone of scout-cli's five direct
/api/chat//api/generatecallers sent a generation cap (grepped for all of them):chat_with_metricsignored its ownmax_tokenson the ollama branch (already honored it for koboldcpp),ollama_chat/_ollama_generatesent no options at all, the fallback direct posts inagent/cli.py/ava/cli.py/write/cli.pyhad the same gap, andscout ollama run --num-predictsent nothing when omitted. This is the same failure mode apps/dottie's PR #66 fixed: with llama-server's--context-shift, an uncapped call doesn't error, it keeps generating (708s / 81,920 tokens, measured there). Addsbigbang.core.llm.resolve_num_predict()— explicit argument >DOTTIE_OLLAMA_NUM_PREDICTenv > 2048 default; 0/negative means no cap — reusing the same env var apps/dottie'sOllamaPolicyreads, so one setting caps every local Ollama caller on the box. A malformed env value raises/fails closed rather than silently running uncapped, mirrored from that same PR.bigbang/core/ollama.py'scomplete()is deliberately untouched — it's documented as pure options passthrough with its own test asserting an omittedoptions=sends none.As a side effect,
scripts/bench_local_runner.py(which already passes an explicit--max-tokenson both backends) had an asymmetry: koboldcpp respected it, ollama silently didn't — so a prior ollama-vs-kobold bench comparison ran ollama uncapped against a capped kobold run. This fixes that.3.
2b4e2a4— Windows-safeherd close --killclose_session's kill path calledos.killpg/signal.SIGKILLunconditionally. Neither exists on Windows at all (os.killpgisn't a module attribute there;signal.SIGKILLisn't defined), so the first line raisedAttributeError, caught by nothing. This was latent until PR #67 made_pid_alivecorrect on Windows — before that,alive()couldn't be trusted enough to route here. Extracts the existing TERM-then-KILL sequence into_kill_session_posix()unchanged, adds_kill_session_win32()usingtaskkill /PID <pid> /T /Fvia subprocess with an argument list (nevershell=True), dispatches onsys.platformthe same way_pid_alivealready does./Tmatters because the recorded pid isstart_session's supervisor, not the real command.Test plan
uv sync --all-groups --frozenin a fresh worktree offorigin/mainuv run pytest tests -qfromapps/scout-cli: 2683 passed, 1 failed, 2 skipped (302s). The one failure is the pre-existingtests/test_loop_plugin.py::test_evaluate_blocked_on_stale_metrics_exits_2(fcntl import inpackages/dottie-loop, Windows-only, not touched by this PR — matches what was flagged going in).factory/tests/test_mission.py::test_cancel_terminates_only_recorded_sleeping_processfails on this Windows box too — it probes liveness withos.kill(pid, 0), the exact CTRL_C_EVENT class of bug PR scout-cli: local models only (remove paid backends), 8b-first pickers, win32-safe liveness #67 fixed forherd/store.py's own_pid_alive, just not yet applied tofactory/. Zero diff from this PR touchesfactory/; flagging for awareness, not fixing here (out of scope).resolve_num_predictprecedence/parsing,chat_with_metrics/ollama_chat/_options()/write's_ollama_chatcap coverage including malformed-env fails-closed (commit 2); both kill branches with fakes (POSIXos.killpg/os.kill/signal.SIGKILLpatched withraising=False— none exist on this Windows dev box either, same reasoningTestPidAlivealready documents), a dispatcher test per platform, and one real end-to-end test that starts a genuine long-running child through the CLI and confirms--kill --forceactually terminates it — this one reproduces the exact pre-fixAttributeErroron this box.uvx ruff@0.15.22 checkon every touched file before/after each commit: 0 delta (pre-existing findings inllm.py,agent/cli.py,ava/cli.py,herd/store.pyunchanged, all pre-dating this PR)scripts/check_documented_counts.py --check: 970 reproduces before and after all three commits — no documented-count update neededscripts/gate_audit.py,check_declared_capabilities.py,check_resolver_fallbacks.py,check_shell_true.py,check_cli_path_args.py,store_symmetry_audit.py— all--checkclean, no new findingsscripts/test_gate_audit.py,test_retrieval_eval.py,test_task_eval_slice.py,test_check_declared_capabilities.py,test_store_symmetry_audit.py,test_dag_next.py— all passscripts/dag_next.py --check,factory/tests(1 pre-existing unrelated Windows failure, see above),python -m factory check— passscripts/check_todos_timestamps.py,scripts/check_handoff_fresh.py --check(10-commit drift, within the 20-commit budget) — passbigbang.cli leaks scan .andleaks history --max-commits 0(1665 commits) — both cleanNotes
Update: review findings addressed (
eec564f)Three minor findings from this PR's review, all confirmed and fixed:
ollama_chat()swallowed a malformedDOTTIE_OLLAMA_NUM_PREDICTinto a genericNone.cap = resolve_num_predict(num_predict)sat inside its own broadexcept Exception: return None, so a typo'd env var was indistinguishable from a down server. Moved the resolve call before the client opens, so the actionableValueErrornow reaches the caller — matching_ollama_generate/chat_with_metrics.Correcting the finding's own evidence: it claimed
_ollama_generate/chat_with_metricsalready contrasted withollama_chatby propagating thatValueErrorintometa['error']. Empirically false — I reproduced it live before touching any code (DOTTIE_OLLAMA_NUM_PREDICT=lotsagainstchat_with_metrics("ollama", ...)returnederror: 'backend returned no completion', not the env var's name)._ollama_generatehad the identical swallow (its ownresolve_num_predict()call was inside the same broad except). Fixed both, and tightenedtest_ollama_chat_with_metrics_a_garbage_cap_fails_closed_not_uncappedto assert the message now namesDOTTIE_OLLAMA_NUM_PREDICT— it failed against pre-fix code, confirming the test exercises something real, not just re-asserting the status quo. Addedtest_ollama_chat_a_garbage_cap_raises_not_silently_noneforollama_chat()itself.Left
ava/cli.py's_route_with_ollamauntouched: it wraps_core_ollama_chatin its own pre-existingexcept Exception: raw = None, soscout avastill degrades to keyword routing on a bad env var rather than surfacing the message — a separate, pre-existing resilience design in that plugin, out of this finding's scope.One more disclosure, found while auditing every real call site of the now-raising
ollama_chat/_ollama_generatefor an unwrapped crash risk (repo-wide grep, not just withinllm.py):bigbang/plugins/harness/executors/llm_exec.py's_ollama()callsllm._ollama_generate(...)directly with no local try/except. It is not agent/ava's_route_with_ollama/_ollama_planner(both already wrap their call inexcept Exception, confirmed) — this one call site propagates theValueErrorup throughcomplete()/run()intobigbang/plugins/harness/runner.py's_dispatch(), whose own except is narrowly(ExecutorUnavailable, NotApplicable)and does not catch it. It does not crash the harness run:_dispatch()'s caller,_attempt(), already wraps every executor call in a genericexcept Exception as exc:as part of the existing per-node recovery ladder (this is the same mechanism any other executor failure — a network timeout, a malformed response — already goes through). The observable difference is which fallback path engages, and only in auto mode: previously a garbageDOTTIE_OLLAMA_NUM_PREDICTdegraded throughExecutorUnavailablestraight to the stub executor with a misleading "returned no completion (runollama pull qwen3:8b)" message; now it's recorded as aTOOL_FAILUREin the recovery ladder witherror_classnaming the actual problem ("ValueError: DOTTIE_OLLAMA_NUM_PREDICT must be an integer, got 'lots'"), which is more actionable, not less. In real mode this is a non-change:_dispatchlines 272-274 already re-raiseExecutorUnavailablethere (if mode == "real" and isinstance(exc, ExecutorUnavailable): raise) into the same genericexcept Exceptionin_attempt, so real mode's path was identical before and after this fix. No test currently exercises this exact interaction (test_harness_executors.pynever sets this env var;conftest.pypops it suite-wide precisely to keep a developer's local env quirks out of the test run), so flagging it here rather than claiming coverage that doesn't exist. Not changingllm_exec.pyto swallow it back to the old behavior — that would be reintroducing the exact "typo degrades silently" failure mode this whole finding exists to close.agent/cli.py's andava/cli.py's fallback duplicates (reachable only when importingbigbang.core.llmhas already failed) had zero coverage, as the finding noted — confirmed_HAS_CORE_LLM/_HAS_LLMareTruein this repo's real environment, so the branch never ran, and the commit that added the cap logic there said as much explicitly ("forcing that import failure reliably under pytest is disproportionate to a defensive fallback"). That turned out to be easy and reliable: added one test per plugin usingsys.modules["bigbang.core.llm"] = None(the documented sys.modules sentinel that forcesImportError, not a monkeypatch of real internals) +importlib.reload, driving the real fallback function with a fake httpx client, restoring and reloading again infinally.bigbang/core/ollama.py'scomplete()applies no default cap of its own — deliberate (documented pure options passthrough with its own test), per the finding correctly identified as no action needed beyond a comment. Added one.Verified:
uv run pytest tests -qfromapps/scout-cli— 2686 passed, 1 failed, 2 skipped (243s; 2686 = this PR's prior 2683 + 3 new tests). The one failure is the pre-existingtests/test_loop_plugin.py::test_evaluate_blocked_on_stale_metrics_exits_2(fcntl import inpackages/dottie-loop, Windows-only, unrelated).check_documented_counts.py --check: 970 reproduces, unchanged.gate_audit.py,goat_audit.py --min-mean 7.0,check_declared_capabilities.py,check_resolver_fallbacks.py,check_shell_true.py,check_cli_path_args.py,store_symmetry_audit.py,dag_next.py,check_todos_timestamps.py,check_handoff_fresh.py,leaks scan .— all clean, no new findings. State-pollution sweep around the new/changed test files: CLEAN.🤖 Generated with Claude Code
https://claude.ai/code/session_014LHC4Z5q8HGJxLCSG3h8Db