diff --git a/openspec/changes/init-model-picker/tasks.md b/openspec/changes/init-model-picker/tasks.md index 67a64f5..7b43094 100644 --- a/openspec/changes/init-model-picker/tasks.md +++ b/openspec/changes/init-model-picker/tasks.md @@ -70,27 +70,27 @@ Chain strategy: sequential-to-main ### Phase 4: Picker implementation — `src/openkos/cli/main.py` -- [ ] 4.1 RED: extend `tests/unit/cli/test_init.py` — `_fake_ollama_client` returns `InstalledModel` list; test numbered list rendered with chat models, `DEFAULT_MODEL` marked "(recommended)". -- [ ] 4.2 GREEN: implement `_pick_chat_model() -> str` — probe Ollama, filter via `is_embedding_model`, ensure `DEFAULT_MODEL` present, print numbered list. -- [ ] 4.3 RED: test Enter (empty input) selects `DEFAULT_MODEL`; test numeric choice maps to corresponding tag. -- [ ] 4.4 GREEN: implement bounded `typer.prompt` reprompt loop mapping empty→recommended, in-range digit→tag. -- [ ] 4.5 RED: test invalid input reprompts until valid; result still passes through `validate_model`. -- [ ] 4.6 GREEN: wire invalid-input reprompt + `validate_model` call on chosen tag. -- [ ] 4.7 RED: test `--model` flag bypasses picker entirely (no probe call). -- [ ] 4.8 GREEN: confirm `_resolve_model` short-circuits before `_pick_chat_model` when `--model` given. -- [ ] 4.9 RED: test non-TTY path silently returns `DEFAULT_MODEL`, no picker invoked. -- [ ] 4.10 GREEN: confirm non-TTY branch bypasses `_pick_chat_model`. -- [ ] 4.11 RED: test Ollama unreachable → falls back to existing typed-prompt/default flow, no hard-fail, workspace still created, exit 0. -- [ ] 4.12 RED: test zero chat models after embedding filter → same fallback behavior. -- [ ] 4.13 GREEN: wrap probe + filter in broad `except Exception`; on exception or zero candidates, fall back to `typer.prompt("Model", default=DEFAULT_MODEL)`. -- [ ] 4.14 RED: test embedding model (e.g. `bge-m3`, family "bert") absent from numbered list while chat model (e.g. `qwen3:8b`) present. -- [ ] 4.15 GREEN: confirm embedding filter applied before rendering list (should already pass from 4.13; add if gap found). +- [x] 4.1 RED: extend `tests/unit/cli/test_init.py` — `_fake_ollama_client` returns `InstalledModel` list; test numbered list rendered with chat models, `DEFAULT_MODEL` marked "(recommended)". +- [x] 4.2 GREEN: implement `_pick_chat_model() -> str` — probe Ollama, filter via `is_embedding_model`, ensure `DEFAULT_MODEL` present, print numbered list. +- [x] 4.3 RED: test Enter (empty input) selects `DEFAULT_MODEL`; test numeric choice maps to corresponding tag. +- [x] 4.4 GREEN: implement bounded `typer.prompt` reprompt loop mapping empty→recommended, in-range digit→tag. +- [x] 4.5 RED: test invalid input reprompts until valid; result still passes through `validate_model`. +- [x] 4.6 GREEN: wire invalid-input reprompt + `validate_model` call on chosen tag. +- [x] 4.7 RED: test `--model` flag bypasses picker entirely (no probe call). +- [x] 4.8 GREEN: confirm `_resolve_model` short-circuits before `_pick_chat_model` when `--model` given. +- [x] 4.9 RED: test non-TTY path silently returns `DEFAULT_MODEL`, no picker invoked. +- [x] 4.10 GREEN: confirm non-TTY branch bypasses `_pick_chat_model`. +- [x] 4.11 RED: test Ollama unreachable → falls back to existing typed-prompt/default flow, no hard-fail, workspace still created, exit 0. +- [x] 4.12 RED: test zero chat models after embedding filter → same fallback behavior. +- [x] 4.13 GREEN: wrap probe + filter in broad `except Exception`; on exception or zero candidates, fall back to `typer.prompt("Model", default=DEFAULT_MODEL)`. +- [x] 4.14 RED: test embedding model (e.g. `bge-m3`, family "bert") absent from numbered list while chat model (e.g. `qwen3:8b`) present. +- [x] 4.15 GREEN: confirm embedding filter applied before rendering list (verified via 4.1's assertion; no gap found). ### Phase 5: Quality Gate (B-ii) -- [ ] 5.1 Run `uv run pytest` — full suite green. -- [ ] 5.2 Run `uv run ruff check . && uv run ruff format --check .`. -- [ ] 5.3 Run `uv run mypy .`. +- [x] 5.1 Run `uv run pytest` — full suite green. +- [x] 5.2 Run `uv run ruff check . && uv run ruff format --check .`. +- [x] 5.3 Run `uv run mypy .`. ## Notes diff --git a/openspec/changes/init-model-picker/verify-report.md b/openspec/changes/init-model-picker/verify-report.md index 85a0694..57019a2 100644 --- a/openspec/changes/init-model-picker/verify-report.md +++ b/openspec/changes/init-model-picker/verify-report.md @@ -1,14 +1,14 @@ ```yaml schema: gentle-ai.verify-result/v1 -evidence_revision: sha256:2f1b38fe9170043ced7492846fe3eb543cd97662192391e1156b9191cccdc232 +evidence_revision: sha256:701ff28d9a3de60a559552fd8c8d10134133bc914ff83891063314b7d70e5b3b verdict: pass blockers: 0 critical_findings: 0 -requirements: 3/3 -scenarios: 10/10 +requirements: 8/8 +scenarios: 27/27 test_command: uv run pytest test_exit_code: 0 -test_output_hash: sha256:d2be0013f8de3a8397ec2dca732a5b8c24cb0031316bfec145d9075ae77ae7bb +test_output_hash: sha256:d0ba5b747fbdaff816f78ca1b40365a3b887aa441fd2041134d65dd79d74f2da build_command: uv run ruff check . && uv run ruff format --check . && uv run mypy . build_exit_code: 0 build_output_hash: sha256:c028c2b916869a306e6c5e3b9656d0fae094dd2afb3689f8fe8158bd1200ba92 @@ -16,102 +16,188 @@ build_output_hash: sha256:c028c2b916869a306e6c5e3b9656d0fae094dd2afb3689f8fe8158 ## Verification Report -**Change**: init-model-picker (Slice B of #128) — Sub-slice B-i only (list_models widening + doctor/preflight adaptation) +**Change**: init-model-picker (Slice B of #128) — FULL change (B-i + B-ii). This slice closes #128. **Version**: N/A **Mode**: Strict TDD -**Scope note**: This report verifies ONLY sub-slice B-i (PR 1 of 2). The interactive model picker (_pick_chat_model, B-ii, tasks 4.x/5.x) is intentionally not implemented yet and is correctly out of scope for this verification. Its absence is not a failure. The workspace-init domain spec's picker-related requirements (Interactive Model Picker, Graceful Degradation, Non-Interactive Bypass, Embedding Exclusion from picker) belong to B-ii and are excluded from the requirement/scenario counts below. +**Scope note**: This report supersedes and extends the prior B-i-only verify report +(previously at `openspec/changes/init-model-picker/verify-report.md`, evidence +`sha256:2f1b38fe9...`, requirements 3/3, scenarios 10/10). B-i (`InstalledModel`, +`is_embedding_model`, widened `list_models()`, doctor/preflight adaptation) is +already merged to `main` at `0652a64 feat(cli): widen list_models to expose +model family (#128, Slice B-i) (#172)` and was NOT re-verified line-by-line here +beyond confirming (a) the working tree diff for this slice touches only +`src/openkos/cli/main.py`, `tests/unit/cli/test_init.py`, and +`openspec/changes/init-model-picker/tasks.md` — B-i's files +(`src/openkos/llm/ollama.py`, `tests/unit/llm/test_ollama.py`, +`tests/unit/cli/test_doctor.py`) are untouched in this diff — and (b) the full +suite (which still includes B-i's tests) remains green with zero regressions. +This report independently re-ran the full suite rather than trusting +apply-progress's self-reported "2106 passed." ### Completeness | Metric | Value | |--------|-------| -| Tasks total (B-i) | 11 (1.1-1.4, 2.1-2.4, 3.1-3.3) | -| Tasks complete (B-i) | 11 | -| Tasks incomplete (B-i) | 0 | -| Tasks remaining (B-ii, out of scope) | 5 (4.1-4.15 grouped, 5.1-5.3) - correctly unchecked | +| Tasks total (both sub-slices) | 20 (1.1-1.4, 2.1-2.4, 3.1-3.3, 4.1-4.15, 5.1-5.3) | +| Tasks complete | 20 | +| Tasks incomplete | 0 | -Verified directly against openspec/changes/init-model-picker/tasks.md on disk: all B-i checkboxes are [x], all B-ii checkboxes are [ ]. +Verified against `openspec/changes/init-model-picker/tasks.md` on disk (working +tree, not yet committed): all 20 checkboxes across both sub-slices are `[x]`. +Matches apply-progress's "20/20 tasks complete" claim. ### Build & Tests Execution (independently re-run, not trusted from apply-progress) -Build: PASSED -- uv run ruff check . -> All checks passed! -- uv run ruff format --check . -> 134 files already formatted -- uv run mypy . -> Success: no issues found in 134 source files +**Build**: PASSED +```text +$ uv run ruff check . +All checks passed! -Tests: 2099 passed / 0 failed / 0 skipped -- uv run pytest -> 2099 passed in 103.19s +$ uv run ruff format --check . +134 files already formatted -Coverage: Not available - no coverage tool detected in project config. Not flagged as a failure per skill rules. +$ uv run mypy . +Success: no issues found in 134 source files +``` + +**Tests**: PASSED +```text +$ uv run pytest +======================= 2106 passed in 103.89s (0:01:43) ======================= +``` +Exit code: 0. Matches apply-progress's reported count (2099 B-i baseline + 7 net +new = 2106) — independently reproduced, not merely copied from the self-report. -### Spec Compliance Matrix (B-i scope: llm-client + doctor-command domains only) +**Coverage**: Not measured (no coverage tool configured in this project's +quality gate) → ➖ Not available. + +### Spec Compliance Matrix + +#### Domain: llm-client (verified already-merged B-i behavior, re-confirmed green) | Requirement | Scenario | Test | Result | -|-------------|----------|------|--------| -| List Installed Models (MODIFIED) | Reachable server returns installed tags with family | test_ollama.py::test_list_models_returns_installed_models_with_tag_and_family | COMPLIANT | -| List Installed Models | Entry missing details/family still returned | test_ollama.py::test_list_models_missing_details_yields_none_family_and_is_kept, ::test_list_models_missing_family_key_yields_none_family | COMPLIANT | -| List Installed Models | Tag extraction preserves model-or-name fallback | test_ollama.py::test_list_models_falls_back_to_name_field | COMPLIANT | -| List Installed Models | Unreachable server raises OllamaUnavailable | test_ollama.py::test_list_models_unreachable_raises_ollama_unavailable, ::test_list_models_body_read_failure_raises_ollama_unavailable | COMPLIANT | -| List Installed Models | Non-200/malformed raises OllamaError | test_ollama.py::test_list_models_non_200_raises_ollama_error, ::test_list_models_malformed_json_raises_ollama_error, ::test_list_models_non_list_models_value_raises_ollama_error | COMPLIANT | -| Family-Based Embedding Model Classification (ADDED) | Known embedding family classifies as embedding | test_ollama.py::test_is_embedding_model_known_embedding_family_returns_true (parametrized: bert, BERT, nomic-bert, Nomic-Bert) | COMPLIANT | -| Family-Based Embedding Model Classification | Missing/unknown family classifies as non-embedding | test_ollama.py::test_is_embedding_model_non_embedding_family_returns_false (parametrized: qwen, llama, unknownfamily), ::test_is_embedding_model_none_family_returns_false | COMPLIANT | -| Doctor Behavior Unchanged (ADDED) | Configured model present still passes | test_doctor.py::test_doctor_model_installed_honors_latest_normalization (+ existing pass-path tests) | COMPLIANT | -| Doctor Behavior Unchanged | Configured model absent still fails with pull remediation | test_doctor.py::test_doctor_missing_model_shows_pull_remediation_with_exact_tag | COMPLIANT | -| Doctor Behavior Unchanged | Embedding-model check outcome unchanged | test_doctor.py::test_doctor_embedding_model_installed_shows_pass, ::test_doctor_embedding_model_missing_shows_pull_remediation_but_exit_zero, ::test_doctor_embedding_model_check_skips_when_ollama_unreachable, ::test_doctor_embedding_model_check_runs_outside_workspace_against_default, ::test_doctor_embedding_model_check_does_not_construct_extra_client | COMPLIANT | - -Compliance summary: 10/10 B-i-scoped scenarios compliant (3 requirements: List Installed Models, Family-Based Embedding Model Classification, Doctor Behavior Unchanged). +|---|---|---|---| +| List Installed Models (MODIFIED) | chat model family + embedding family both returned with family | `tests/unit/llm/test_ollama.py` | ✅ COMPLIANT | +| List Installed Models | entry missing details/family still returned | `tests/unit/llm/test_ollama.py` | ✅ COMPLIANT | +| List Installed Models | name-fallback tag extraction preserved | `tests/unit/llm/test_ollama.py` | ✅ COMPLIANT | +| List Installed Models | unreachable -> OllamaUnavailable | `tests/unit/llm/test_ollama.py` | ✅ COMPLIANT | +| List Installed Models | non-200/malformed -> OllamaError | `tests/unit/llm/test_ollama.py` | ✅ COMPLIANT | +| Family-Based Embedding Classification (ADDED) | known embedding family -> embedding | `tests/unit/llm/test_ollama.py` | ✅ COMPLIANT | +| Family-Based Embedding Classification | missing/unknown family -> non-embedding | `tests/unit/llm/test_ollama.py` | ✅ COMPLIANT | -### Correctness (Static Evidence) -| Requirement | Status | Notes | -|------------|--------|-------| -| InstalledModel dataclass | Implemented | frozen=True, slots=True, tag: str, family: str or None, matches design D1 exactly | -| is_embedding_model() | Implemented | _EMBEDDING_FAMILIES = frozenset({"bert","nomic-bert"}), case-insensitive via .lower(), None family -> False (never excludes on ambiguity), matches design D2 exactly | -| list_models() widened | Implemented | Returns list[InstalledModel]; D2 tag fallback (model or name) preserved; guarded isinstance(details, dict) before .get("family") - a defensive hardening beyond the design's literal snippet, correctly handles explicit "details": null without crashing (verified: this exact edge case is not covered by a dedicated RED test, see Issues) | -| model_tag_matches() signature | Unchanged | Still (configured: str, installed: list[str]) -> bool - confirmed via grep, no caller passes InstalledModel objects directly | -| Every list_models() call site updated | Confirmed | 3 production call sites (main.py:344 init preflight, main.py:5564 doctor check 3, run_spike.py:739) all extract .tag; no caller left consuming the old list[str] shape | -| Doctor checks 3/4/5 adapted | Implemented | installed_tags = [m.tag for m in installed] built once after check 3, reused by checks 4/5, matches design D4 exactly | -| No picker code added | Confirmed | grep -rn "_pick_chat_model or picker" across src/ and tests/ returns zero matches - B-ii correctly not started | +#### Domain: doctor-command (verified already-merged B-i behavior, re-confirmed green) -### Coherence (Design) -| Decision | Followed? | Notes | -|----------|-----------|-------| -| D1 - widened return type to list[InstalledModel] dataclass | Yes | Exact match | -| D2 - embedding classification via _EMBEDDING_FAMILIES frozenset | Yes | Exact match, case-insensitive | -| D4 - doctor/preflight minimal adaptation, model_tag_matches unchanged | Yes | Exact match | -| Sizing - B-i ~215 lines forecast | Yes | Actual diff: 133 additions + 31 deletions = 164 authored changed lines across 6 files, under forecast and well under the 400/800-line budgets | +| Requirement | Scenario | Test | Result | +|---|---|---|---| +| Doctor Behavior Unchanged (ADDED) | configured model present -> [PASS] unchanged | `tests/unit/cli/test_doctor.py` | ✅ COMPLIANT | +| Doctor Behavior Unchanged | configured model absent -> [FAIL] unchanged | `tests/unit/cli/test_doctor.py` | ✅ COMPLIANT | +| Doctor Behavior Unchanged | embedding-model check outcome unchanged | `tests/unit/cli/test_doctor.py` | ✅ COMPLIANT | -### Issues Found +#### Domain: workspace-init (this slice, B-ii — newly verified) -CRITICAL: None +| Requirement | Scenario | Test | Result | +|---|---|---|---| +| Static openkos.yaml Template (MODIFIED) | Byte-identical except model, default path | `test_non_tty_no_flag_silent_default` | ✅ COMPLIANT | +| Static openkos.yaml Template | Flag override selects the model | `test_model_flag_writes_chosen_model`, `test_model_flag_with_colon_tag_writes_verbatim` | ✅ COMPLIANT | +| Static openkos.yaml Template | TTY, picker preconditions hold, accept default | `test_tty_prompt_accepts_default` | ✅ COMPLIANT | +| Static openkos.yaml Template | TTY, picker preconditions hold, custom selection | `test_tty_prompt_custom_value`, `test_picker_numeric_choice_selects_and_persists` | ✅ COMPLIANT | +| Static openkos.yaml Template | Non-TTY, no flag, silent default | `test_non_tty_no_flag_silent_default` | ✅ COMPLIANT | +| Static openkos.yaml Template | Blank input is rejected | `test_model_flag_rejects_blank_or_unsafe_value[...]` | ✅ COMPLIANT | +| Static openkos.yaml Template | Unsafe token is rejected | `test_model_flag_rejects_blank_or_unsafe_value[...]` | ✅ COMPLIANT | +| Static openkos.yaml Template | Reserved YAML boolean/null word rejected, case-insensitive | `tests/unit/test_config.py::test_validate_model_rejects_yaml_reserved_words` (unit-level, reused by picker path via shared `config.validate_model`) | ✅ COMPLIANT | +| Interactive Model Picker Over Installed Chat Models (ADDED) | Picker lists installed chat models with default marked | `test_picker_lists_chat_models_excludes_embedding` | ✅ COMPLIANT | +| Interactive Model Picker | Selecting a number picks that model | `test_picker_numeric_choice_selects_and_persists` | ✅ COMPLIANT | +| Interactive Model Picker | Empty input picks the default | `test_tty_prompt_accepts_default` | ✅ COMPLIANT | +| Interactive Model Picker | Selection is persisted to openkos.yaml | `test_picker_numeric_choice_selects_and_persists` | ✅ COMPLIANT | +| Graceful Degradation When Ollama Unreachable Or No Chat Models (ADDED, CRITICAL) | Unreachable Ollama falls back, workspace still created | `test_picker_unreachable_ollama_falls_back_to_typed_prompt` | ✅ COMPLIANT | +| Graceful Degradation | Only embedding models installed falls back, no crash | `test_picker_zero_chat_models_falls_back_to_typed_prompt` | ✅ COMPLIANT | +| Non-Interactive Paths Bypass The Picker (ADDED) | --model flag wins, no picker even on a TTY | `test_model_flag_wins_over_tty_prompt`, `test_model_flag_bypasses_picker_no_list_shown` | ✅ COMPLIANT | +| Non-Interactive Paths Bypass The Picker | Non-TTY silently takes the default, no picker | `test_non_tty_no_flag_silent_default`, `test_non_tty_bypasses_picker_silent_default` | ✅ COMPLIANT | +| Embedding Models Excluded From Picker Candidates (ADDED) | Embedding model never offered as a picker choice | `test_picker_lists_chat_models_excludes_embedding` | ✅ COMPLIANT | + +**Compliance summary**: 27/27 scenarios compliant (10/10 pre-existing B-i domains +re-confirmed green, 17/17 workspace-init scenarios newly verified for B-ii). -WARNING: None +### Correctness (Static Evidence) + +| Requirement | Status | Notes | +|---|---|---| +| `_pick_chat_model()` | ✅ Implemented | Probes via `OllamaClient(model=config.DEFAULT_MODEL, timeout=_PREFLIGHT_TIMEOUT)`, filters `is_embedding_model`, ensures `DEFAULT_MODEL` present and first, `(recommended)` suffix | +| `_resolve_model` TTY branch | ✅ Implemented | Delegates to `_pick_chat_model()`; `--model` flag and non-TTY branches unchanged and still short-circuit before it | +| Bounded reprompt | ✅ Implemented | `_MAX_PICKER_ATTEMPTS = 3` loop; invalid/non-numeric/out-of-range choice reprompts with stderr guidance; loop exit falls back to `config.validate_model(config.DEFAULT_MODEL)` — no infinite loop, no hang | +| `validate_model` applied to picker result | ✅ Implemented | Both the numeric-selection path (`candidates[int(choice)-1]`) and the exhausted-fallback path pass through `config.validate_model` before returning | -SUGGESTION: -- The apply-progress artifact documents a defensive-hardening deviation from the design's literal entry.get("details", {}).get("family") snippet: an added isinstance(details, dict) guard to handle an explicit "details": null entry without raising AttributeError. This is sound and behavior-preserving (confirmed by code inspection), but there is no explicit RED test exercising "details": null specifically (the existing "missing details key" test covers key-absence, not explicit-null). Low priority - recommend adding one parametrized case in a follow-up if this edge case is expected with real Ollama servers. -- No coverage tool is configured, so changed-file line/branch coverage cannot be independently confirmed beyond scenario-level test mapping above (informational only, not blocking per Strict TDD rules). +### Coherence (Design) + +| Decision | Followed? | Notes | +|---|---|---| +| D3 picker delegation from `_resolve_model` TTY branch | ✅ Yes | `--model` and non-TTY still short-circuit before `_pick_chat_model` is ever called | +| D3 broad `except Exception` around probe+filter | ✅ Yes | Matches design; unreachable Ollama and zero-candidates-after-filter share one fallback path (task 4.13) | +| D3 `typer.prompt(..., default="1")` instead of manual empty-string branch | ✅ Yes (documented deviation) | apply-progress explicitly notes this as a non-material implementation detail; functionally identical to design's "empty→recommended tag" — confirmed correct by `test_tty_prompt_accepts_default` | +| D3 reuse of `_PREFLIGHT_TIMEOUT` instead of a second timeout constant | ✅ Yes (documented deviation) | Matches design's "mirrors post-write preflight tolerance" note | +| B-ii depends on B-i's merged `InstalledModel`/`is_embedding_model` | ✅ Yes | Diff touches zero B-i files; `import is_embedding_model` from already-merged `openkos.llm.ollama` | ### TDD Compliance | Check | Result | Details | -|-------|--------|---------| -| TDD Evidence reported | Yes | "TDD Cycle Evidence" table found in apply-progress (Engram #1912) | -| All tasks have tests | Yes | 11/11 B-i checklist items map to test changes in test_ollama.py, test_doctor.py, test_init.py | -| RED confirmed (tests exist) | Yes | All referenced test files exist and contain the described test functions (verified via git diff inspection) | -| GREEN confirmed (tests pass) | Yes | 2099/2099 tests pass on independent re-run | -| Triangulation adequate | Yes | is_embedding_model triangulated via pytest.mark.parametrize (4 embedding-family cases, 3 non-embedding cases, 1 None case); list_models() triangulated across 4 distinct scenario tests (family-present, missing-details, missing-family-key, name-fallback) | -| Safety Net for modified files | Yes | test_doctor.py/test_init.py fake stubs modified in place; full existing suites re-run green after the change, confirmed by the 2099-pass count including pre-existing doctor/init tests | - -TDD Compliance: 6/6 checks passed +|---|---|---| +| TDD Evidence reported | ✅ | "TDD Cycle Evidence" table present in apply-progress (#1912) for B-ii | +| All tasks have tests | ✅ | 15/15 B-ii checklist items map to test files/assertions in `tests/unit/cli/test_init.py` | +| RED confirmed (tests exist) | ✅ | All 8 new picker tests plus 2 repurposed tests exist in `tests/unit/cli/test_init.py` (verified by direct read) | +| GREEN confirmed (tests pass) | ✅ | 2106/2106 pass on independent re-run, exit 0 | +| Triangulation adequate | ✅ | Distinct test cases per behavior (list rendering, numeric selection, empty-input default, invalid-then-valid reprompt, flag bypass, non-TTY bypass, unreachable fallback, zero-candidates fallback, embedding exclusion) | +| Safety Net for modified files | ✅ | `src/openkos/cli/main.py` and `tests/unit/cli/test_init.py` modified; full B-i baseline (2099 tests) re-ran and stayed green | + +**TDD Compliance**: 6/6 checks passed + +### Test Layer Distribution +| Layer | Tests | Files | Tools | +|---|---|---|---| +| Unit | 8 new + 2 repurposed (picker) | 1 (`tests/unit/cli/test_init.py`) | pytest + `typer.testing.CliRunner` with a mocked `OllamaClient` (no real HTTP/subprocess) | +| Integration | 0 | 0 | — | +| E2E | 0 | 0 | — | +| **Total (B-ii)** | **10** | **1** | | ### Assertion Quality -No violations found in the diffed test files. All new/modified assertions are direct value comparisons against InstalledModel instances or boolean classification results (assert result == [InstalledModel(...)], assert is_embedding_model(model) is True/False) - no tautologies, no empty-collection-only checks, no ghost loops over possibly-empty collections, no smoke-test-only patterns, no implementation-detail coupling. - -Assertion quality: All assertions verify real behavior +✅ All assertions verify real behavior — no tautologies, no assertion-free tests, +no ghost loops, no smoke-test-only patterns found in the reviewed diff. Every +new test asserts either `result.exit_code`, presence/absence of specific +strings in CLI output (`"(recommended)"`, model tags), or the exact persisted +`model:` line in `openkos.yaml`. ### Quality Metrics -Linter: No errors (uv run ruff check .) -Formatter: No diffs (uv run ruff format --check .) -Type Checker: No errors (uv run mypy ., 134 source files) +**Linter**: ✅ No errors (`ruff check .` — all checks passed) +**Type Checker**: ✅ No errors (`mypy .` — no issues in 134 source files) + +### Issues Found + +**CRITICAL**: None. + +**WARNING**: +- The bounded-reprompt *exhaustion* path (all `_MAX_PICKER_ATTEMPTS = 3` + attempts invalid, falling back to `config.validate_model(config.DEFAULT_MODEL)` + without hanging or crashing) is implemented correctly — I independently + reproduced it with an ad hoc pytest case (3x invalid input, exit 0, falls + back to `qwen3:8b`, no exception) — but this exact scenario is **not** + covered by a shipped test in `tests/unit/cli/test_init.py`. + `test_picker_invalid_selection_reprompts_then_succeeds` only exercises one + invalid attempt followed by a valid one (2 of 3 available attempts), not + the exhausted-fallback branch itself. This is not a formal spec scenario + (the spec text does not enumerate an "attempts exhausted" case, only + design D3's prose), so it is not a spec-compliance failure, but it is a + real coverage gap for a documented safety behavior and should be closed + with a follow-up test before this code path changes again. + +**SUGGESTION**: None. ### Verdict -PASS -All 11 B-i tasks genuinely complete and match code state; all 3 B-i-scoped spec requirements (10/10 scenarios) covered by passing tests; independently re-run full quality gate (pytest, ruff check, ruff format, mypy) is green with zero deviation from the apply self-report; no picker code present (correctly deferred to B-ii); diff size (164 authored lines) is well within the review budget. +**PASS** + +All 8 spec requirements / 27 scenarios across the three touched domains +(llm-client, doctor-command, workspace-init) are compliant with a passing +covering test, the full suite (2106 tests) and full quality gate +(ruff check, ruff format --check, mypy) pass cleanly on independent re-run, +all 20/20 tasks across both sub-slices are complete and match the code state, +B-i is confirmed merged to `main` and unaffected by this diff, and no +regression was found. One WARNING (missing explicit test for the bounded- +reprompt exhaustion fallback path) is non-blocking and does not gate closing +#128. diff --git a/src/openkos/cli/main.py b/src/openkos/cli/main.py index 04deb78..50d363b 100644 --- a/src/openkos/cli/main.py +++ b/src/openkos/cli/main.py @@ -37,6 +37,7 @@ OllamaError, OllamaModelNotFound, OllamaUnavailable, + is_embedding_model, model_tag_matches, ) from openkos.model import okf, types @@ -86,6 +87,12 @@ # the three verbs cannot drift from each other in wording. _DOCTOR_HINT = " Or run `openkos doctor` to diagnose the environment." +_MAX_PICKER_ATTEMPTS = 3 +"""Bounded reprompt count for `_pick_chat_model`'s numeric-choice loop -- +an invalid answer reprompts up to this many times before the picker gives +up and silently falls back to `config.DEFAULT_MODEL`, so a non-interactive +or misbehaving stdin can never hang `init` forever (design D3).""" + # Uniform lock-contention message for `reindex`'s two error ladders # (vectors/fts and graph) -- a single source of truth so a locked # vectors.db/fts.db/graph.db always reads identically regardless of which @@ -102,24 +109,97 @@ def callback() -> None: def _resolve_model(flag: str | None) -> str: - """Resolve the model tag to write, precedence flag > TTY prompt > default. + """Resolve the model tag to write, precedence flag > interactive picker > default. `flag` (already the raw `--model` value, or `None` if not given) wins - outright -- no prompt is shown even on a TTY. Otherwise, if stdin is a - TTY, `typer.prompt` offers `config.DEFAULT_MODEL` and the user may - accept it or type a different tag. If stdin is not a TTY (e.g. piped, - or a non-interactive CI run), no prompt is shown and the default is - used silently. Every path runs through `config.validate_model`, which - raises `ValueError` for a blank or unsafe value -- callers must catch - it before any file is written. + outright -- no prompt or picker is shown even on a TTY. Otherwise, if + stdin is a TTY, `_pick_chat_model` probes Ollama and offers a numbered + list of installed chat models (falling back to a typed prompt if the + probe fails or no chat model is installed). If stdin is not a TTY (e.g. + piped, or a non-interactive CI run), neither prompt nor picker is shown + and the default is used silently. Every path runs through + `config.validate_model`, which raises `ValueError` for a blank or unsafe + value -- callers must catch it before any file is written. """ if flag is not None: return config.validate_model(flag) if sys.stdin.isatty(): + return _pick_chat_model() + return config.DEFAULT_MODEL + + +def _is_selectable_model_tag(tag: str) -> bool: + """`True` iff `tag` would pass `config.validate_model` -- used to drop a + server-reported tag the picker could otherwise list but that would + hard-fail `init` the moment the user selected it.""" + try: + config.validate_model(tag) + return True + except ValueError: + return False + + +def _pick_chat_model() -> str: + """Interactive numbered picker over Ollama's installed chat models. + + Probes Ollama in Phase A, strictly before any workspace write, wrapped + in a broad `except Exception` so an unreachable server (or any other + probe failure) never blocks `init` -- it silently falls back to the + pre-picker typed prompt below instead (spec: Graceful Degradation). + Embedding models (per `is_embedding_model`) are excluded from the + candidate list; if that leaves zero chat models, the picker falls back + the same way. `config.DEFAULT_MODEL` is always listed first, marked + "(recommended)" -- prepended if the probe didn't report it installed. + + `typer.prompt("Model", default=str())` reads the + choice: pressing Enter re-supplies that default, so it resolves to the + recommended tag with no special-casing needed. An in-range digit picks + that list entry; anything else reprompts, up to `_MAX_PICKER_ATTEMPTS` + times, rather than silently accepting garbage or hanging forever on a + misbehaving/non-interactive stdin -- after which it falls back to + `config.DEFAULT_MODEL`. The final choice is still validated by + `config.validate_model`, same as every other `_resolve_model` path. + """ + try: + probe = OllamaClient(model=config.DEFAULT_MODEL, timeout=_PREFLIGHT_TIMEOUT) + candidates = [ + m.tag + for m in probe.list_models() + if not is_embedding_model(m) and _is_selectable_model_tag(m.tag) + ] + except Exception: + candidates = [] + + if not candidates: return config.validate_model( typer.prompt("Model", default=config.DEFAULT_MODEL) ) - return config.DEFAULT_MODEL + + if config.DEFAULT_MODEL in candidates: + candidates.remove(config.DEFAULT_MODEL) + candidates.insert(0, config.DEFAULT_MODEL) + + typer.echo("Installed chat models:") + for index, tag in enumerate(candidates, start=1): + suffix = " (recommended)" if tag == config.DEFAULT_MODEL else "" + typer.echo(f" {index}) {tag}{suffix}") + + for _ in range(_MAX_PICKER_ATTEMPTS): + choice = typer.prompt("Model", default="1") + if ( + choice.isascii() + and choice.isdigit() + and 1 <= int(choice) <= len(candidates) + ): + return config.validate_model(candidates[int(choice) - 1]) + typer.echo( + f"openkos init: '{choice}' isn't a valid choice -- enter a " + f"number from 1 to {len(candidates)}, or press Enter for the " + "recommended model.", + err=True, + ) + + return config.validate_model(config.DEFAULT_MODEL) def _commit_has_confidential(root: Path, paths: Sequence[str]) -> bool: diff --git a/tests/unit/cli/test_init.py b/tests/unit/cli/test_init.py index 49d54e3..f77ae5f 100644 --- a/tests/unit/cli/test_init.py +++ b/tests/unit/cli/test_init.py @@ -30,12 +30,19 @@ def _fake_ollama_client( - *, installed: list[str] | None = None, error: Exception | None = None + *, + installed: list[str | InstalledModel] | None = None, + error: Exception | None = None, ) -> Callable[..., Any]: """Build a fake `OllamaClient` factory exposing ONLY `list_models` -- mirrors `test_doctor.py`'s stub. Deliberately has no `pull`/`serve`-style - method, so any attempt by the preflight to call one raises - `AttributeError` and fails the test loudly (task 2.6 guard).""" + method, so any attempt by the preflight or picker to call one raises + `AttributeError` and fails the test loudly (task 2.6 guard). + + `installed` entries may be a bare `str` (shorthand for a chat model with + `family=None`, used by every pre-picker test) or a full `InstalledModel` + (needed by the B-ii picker tests to set `family` and exercise the + embedding-model filter).""" class _FakeOllamaClient: def __init__(self, model: str, **kwargs: object) -> None: @@ -44,7 +51,12 @@ def __init__(self, model: str, **kwargs: object) -> None: def list_models(self) -> list[InstalledModel]: if error is not None: raise error - return [InstalledModel(tag=tag, family=None) for tag in (installed or [])] + return [ + entry + if isinstance(entry, InstalledModel) + else InstalledModel(tag=entry, family=None) + for entry in (installed or []) + ] return _FakeOllamaClient @@ -355,7 +367,11 @@ def test_model_flag_wins_over_tty_prompt( def test_tty_prompt_accepts_default( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """No `--model` flag, stdin is a TTY, user accepts the offered default (scenario: TTY prompt, accept default).""" + """No `--model` flag, stdin is a TTY, Ollama reachable with the default + chat model installed: the picker's numbered list is shown and pressing + Enter selects `config.DEFAULT_MODEL` (scenario: TTY picker, accept + recommended default). Relies on the module's autouse fixture, which + reports `DEFAULT_MODEL` installed.""" monkeypatch.chdir(tmp_path) _simulate_tty(monkeypatch) @@ -392,11 +408,19 @@ def test_tty_init_prints_exact_next_step_hint( def test_tty_prompt_custom_value( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """No `--model` flag, stdin is a TTY, user enters a custom value (scenario: TTY prompt, custom value).""" + """No `--model` flag, stdin is a TTY, Ollama reports a second installed + chat model: the user picks it by NUMBER, not free text (scenario: + TTY+picker custom selection). The picker only accepts a numeric index + (or Enter for the recommended default) -- superseded from B-i's + free-text-prompt version of this test now that the picker is active.""" monkeypatch.chdir(tmp_path) _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client(installed=[DEFAULT_MODEL, "mistral"]), + ) - result = runner.invoke(app, ["init"], input="mistral\n") + result = runner.invoke(app, ["init"], input="2\n") assert result.exit_code == 0 content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") @@ -417,6 +441,247 @@ def test_non_tty_no_flag_silent_default( assert "model: qwen3:8b" in content +# --- Slice B (init-model-picker): interactive chat-model picker ------------ + + +def test_picker_lists_chat_models_excludes_embedding( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The picker's numbered list shows installed chat models and marks the + recommended default, but never lists an embedding model (family + "bert") as a selectable option (spec: Embedding Models Excluded From + Picker Candidates).""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client( + installed=[ + InstalledModel(tag=DEFAULT_MODEL, family="qwen"), + InstalledModel(tag="bge-m3", family="bert"), + ] + ), + ) + + result = runner.invoke(app, ["init"], input="\n") + + assert result.exit_code == 0 + assert "qwen3:8b" in result.output + assert "(recommended)" in result.output + assert "bge-m3" not in result.output + + +def test_picker_numeric_choice_selects_and_persists( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Choosing a non-default chat model by its list number persists that + tag to `openkos.yaml` (spec: selecting a number picks that model).""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client( + installed=[ + InstalledModel(tag=DEFAULT_MODEL, family="qwen"), + InstalledModel(tag="gemma3", family="gemma"), + ] + ), + ) + + result = runner.invoke(app, ["init"], input="2\n") + + assert result.exit_code == 0 + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert "model: gemma3" in content + + +def test_model_flag_bypasses_picker_no_list_shown( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`--model` wins outright even on a TTY: no picker list is rendered at + all (spec: --model on TTY -> no picker).""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client(installed=[DEFAULT_MODEL, "gemma3"]), + ) + + result = runner.invoke(app, ["init", "--model", "mistral"]) + + assert result.exit_code == 0 + assert "(recommended)" not in result.output + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert "model: mistral" in content + + +def test_non_tty_bypasses_picker_silent_default( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Non-TTY, no `--model` flag: the default is taken silently, no picker + list is shown, even though Ollama reports multiple chat models (spec: + non-TTY no flag -> silent default, no picker).""" + monkeypatch.chdir(tmp_path) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client(installed=[DEFAULT_MODEL, "gemma3"]), + ) + + result = runner.invoke(app, ["init"]) + + assert result.exit_code == 0 + assert "(recommended)" not in result.output + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert "model: qwen3:8b" in content + + +def test_picker_unreachable_ollama_falls_back_to_typed_prompt( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Ollama unreachable during the picker's probe: falls back to the + typed-prompt/default flow, never hard-fails, and the workspace is still + created with exit 0 (spec: Graceful Degradation -- unreachable).""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client(error=OllamaUnavailable("Ollama not reachable")), + ) + + result = runner.invoke(app, ["init"], input="\n") + + assert result.exit_code == 0 + assert "(recommended)" not in result.output + assert (tmp_path / "openkos.yaml").is_file() + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert "model: qwen3:8b" in content + + +def test_picker_zero_chat_models_falls_back_to_typed_prompt( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Only embedding models installed (zero chat candidates after the + family filter): the picker falls back to the typed-prompt/default flow + without crashing (spec: Graceful Degradation -- only embedding models + installed).""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client(installed=[InstalledModel(tag="bge-m3", family="bert")]), + ) + + result = runner.invoke(app, ["init"], input="\n") + + assert result.exit_code == 0 + assert "(recommended)" not in result.output + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert "model: qwen3:8b" in content + + +def test_picker_invalid_selection_reprompts_then_succeeds( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """An out-of-range/non-numeric first answer reprompts instead of + crashing or silently accepting garbage; a valid follow-up answer then + completes the picker (spec design D3: invalid -> stderr err + reprompt).""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client( + installed=[ + InstalledModel(tag=DEFAULT_MODEL, family="qwen"), + InstalledModel(tag="gemma3", family="gemma"), + ] + ), + ) + + result = runner.invoke(app, ["init"], input="not-a-number\n2\n") + + assert result.exit_code == 0 + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert "model: gemma3" in content + + +def test_picker_exhausted_invalid_selections_falls_back_to_default( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The reprompt loop is bounded: after `_MAX_PICKER_ATTEMPTS` invalid + answers the picker stops reprompting and falls back to the recommended + default instead of looping forever on a misbehaving stdin (spec: + Graceful Degradation / bounded reprompt).""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client( + installed=[ + InstalledModel(tag=DEFAULT_MODEL, family="qwen"), + InstalledModel(tag="gemma3", family="gemma"), + ] + ), + ) + + result = runner.invoke(app, ["init"], input="bad\nbad\nbad\n") + + assert result.exit_code == 0 + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert f"model: {DEFAULT_MODEL}" in content + + +def test_picker_unicode_digit_input_reprompts( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A Unicode digit-like character (e.g. superscript two, U+00B2) passes + `str.isdigit()` but is not `int()`-parseable. It must be treated as an + invalid choice and reprompt, never escape as a raw `int()` ValueError + that hard-fails init (bounded-reprompt / graceful-degradation contract).""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client( + installed=[ + InstalledModel(tag=DEFAULT_MODEL, family="qwen"), + InstalledModel(tag="gemma3", family="gemma"), + ] + ), + ) + + result = runner.invoke(app, ["init"], input="²\n2\n") + + assert result.exit_code == 0 + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert "model: gemma3" in content + + +def test_picker_excludes_tags_that_fail_validation( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A server-reported chat tag that would fail `validate_model` (e.g. one + containing a space) must not be offered as a selectable option, so a user + can never pick a listed entry that then hard-fails init on validation.""" + monkeypatch.chdir(tmp_path) + _simulate_tty(monkeypatch) + monkeypatch.setattr( + "openkos.cli.main.OllamaClient", + _fake_ollama_client( + installed=[ + InstalledModel(tag=DEFAULT_MODEL, family="qwen"), + InstalledModel(tag="bad tag", family="qwen"), + ] + ), + ) + + result = runner.invoke(app, ["init"], input="\n") + + assert result.exit_code == 0 + assert "bad tag" not in result.output + content = (tmp_path / "openkos.yaml").read_text(encoding="utf-8") + assert f"model: {DEFAULT_MODEL}" in content + + @pytest.mark.parametrize("bad_model", ["", " ", "a b", 'a"b', "a'b", "a#b"]) def test_model_flag_rejects_blank_or_unsafe_value( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, bad_model: str