fix: what the second end-to-end run found

.github/workflows/ci.yml

@@ -14,10 +14,11 @@ concurrency:
 
 jobs:
   behaviour-gate-evidence:
-    # Four of this product's behaviours are produced by a prompt, not by code:
+    # Five of this product's behaviours are produced by a prompt, not by code:
     # an injected Memory constraint changing an answer, a Memory ↔ KB
     # contradiction being reported, a distilled Skill keeping its dead ends and
-    # its blanks, and an opted-in playbook proposing only read-only diagnostics.
+    # its blanks, an opted-in playbook proposing only read-only diagnostics, and
+    # the assistant offering a standing fact to Memory.
     # A reworded instruction can stop any of them, and none of them raises.
     #
     # `make test-behaviour` checks them by calling hosted models, which cannot
@@ -42,12 +43,18 @@ jobs:
           # Deliberately file-level and over-triggering. Being asked to run the
           # gate unnecessarily costs minutes; missing the change it exists for
           # costs the reason it exists.
+          #
+          # retrieval.py holds no prompt. It decides which chunks reach one, and
+          # #203 shipped a change that silently made hybrid search pure-vector
+          # for the shape of question this product mostly gets — every behaviour
+          # below is judged on evidence this file selects.
           PROTECTED='
           src/opspilot/orchestrator/chat_agent.py
           src/opspilot/orchestrator/ticket_summary.py
           src/opspilot/memory/inject.py
           src/opspilot/consultation/distil.py
           src/opspilot/sandbox/proposals.py
+          src/opspilot/kb/retrieval.py
           docs/specs/orchestrator/schemas/incident_summary_v1.schema.json
           '
           CHANGED=$(git diff --name-only origin/${{ github.base_ref }}...HEAD)
@@ -70,7 +77,7 @@ jobs:
           echo ""
           echo "Run it and paste the result into the PR body:"
           echo "    make test-behaviour"
-          echo "    behaviour-gate: 4/4 passed (votes 3/3, 3/3, 3/3, 3/3)"
+          echo "    behaviour-gate: 6 passed (votes 3/3, 3/3, 3/3, 3/3, 3/3, 3/3)"
           echo ""
           echo "It is not run here on purpose — see this job's comment in ci.yml."
           exit 1

Makefile

@@ -54,7 +54,7 @@ test-cov: ensure-venv ## Run tests with coverage.
 	$(PYTEST) tests/ --cov=opspilot --cov-report=term-missing -m "not slow and not requires_ollama and not requires_api_key"
 
 test-behaviour: ensure-venv ## Behaviour gate — does the prompt still drive the behaviour? (needs ANTHROPIC_API_KEY).
-	@echo "Four model-facing behaviours, best of 3. Paste the summary into the PR body."
+	@echo "Five model-facing behaviours in six cases, best of 3. Paste the summary into the PR body."
 	$(PYTEST) tests/ -m "requires_api_key" -p no:randomly
 
 test-ollama: ensure-venv ## Run tests that require a running Ollama (assumes `make ollama-up && make ollama-pull`).

ROADMAP.md

@@ -117,16 +117,57 @@ Ollama and a stub target had silently moved; the behaviour gate scored the new
 Memory-proposal hint at 1/3, which turned out to be `finish_reason: length`
 truncating the label off the end of the answer rather than the model ignoring it.
 
-- **Open decision:** `kb/retrieval.py` is not on the behaviour gate's protected
-  path list, so [#203](https://github.com/vicenteliu/OpsPilot/pull/203) shipped
-  without gate evidence — yet it changes what context reaches the model. Whether
-  retrieval belongs on that list is a decision, not a bug fix, and was left to be
-  made deliberately.
-- **Not re-run since.** All nine fixes changed behaviour on that same path. The
-  way to check them is the way they were found: use it again.
+**Re-run the same day, and it paid again.** All nine fixes were checked the way
+they were found — a fresh `OPSPILOT_HOME`, the README's own commands, hosted
+models. Ten checks, ten passes: the two entry points now agree on an embedder, a
+dot-directory and a `../` path both ingest, a sentence-shaped question gets FTS
+ranks, Haiku and Sonnet 5 price to the cent and OpenRouter reports what it
+billed, the proposal hint fires without repeating itself, `working_set_id`
+survives the API, the bundle round-trips into an empty home with its actors
+intact, `distil --model anthropic/claude-sonnet-5` crosses providers, and the
+model-list editor rewrites a playbook without losing a comment.
+
+Three more defects came out of it, and two are the same shape as the first nine —
+two correct pieces of work that had never been run against each other:
+
+- **The README's own first command failed 5 of 15 files.**
+  `examples/sample_data_en/kb/` was built in May as the sample input for
+  `kb load-dir`, which reads `doc-meta.json` + `chunks.jsonl` pairs; `README.md`
+  later pointed `opspilot ingest` at the same directory. The `.jsonl` files
+  raised `AdapterError`, and the five metadata sidecars were ingested *as
+  documents* — two of them ranked in the top five for "why would a pod be stuck
+  in CrashLoopBackOff". One directory feeding two commands, neither wrong on its
+  own. **Fixed** by splitting it: source documents stay in `kb/`, their frozen
+  projections move to `fixtures/`. `ingest examples/sample_data_en/kb/` now
+  reports `5 succeeded · 0 failed` and retrieves nothing but SOP prose;
+  `kb load-dir examples/sample_data_en/fixtures/` still loads all five pairs
+  with their hand-authored ids intact. No product code changed.
+- **A tool loop that exhausts its rounds answered with its own preamble.**
+  `chat_agent.py` fell back to the last round's `resp.content`, but the last
+  round produced a tool call, so the content was "Let me check the knowledge
+  base…". Observed on a real question: six `kb_search` rounds, three of them the
+  same query, 578 output tokens billed, and 62 characters delivered with seven
+  citations attached to them. **Fixed:** the cap bounds the *tool* rounds, and
+  one final round with no tools turns what was already retrieved into an answer.
+  The model keeps repeating a query it has already run — that is a separate,
+  cheaper problem, and it no longer costs the user an answer.
+- **`propose_actions` is opt-in and nothing opts in** — see the proposed-actions
+  section below.
+
+And one rough edge: the CLI writes as `cli:<osuser>` while the loopback API
+writes as `local-dev`, so `opspilot workingset status` reports nothing open
+while the web UI has a set open for the same person. `distil` escapes it by
+taking an explicit id; `status` and `close` do not.
+
+**Decided:** `kb/retrieval.py` joins the behaviour gate's protected paths.
+[#203](https://github.com/vicenteliu/OpsPilot/pull/203) shipped without gate
+evidence, and what it changed was which chunks reach the model at all — the
+input every one of the five prompt-driven behaviours is judged on. The list's
+own rule already settles it: over-triggering costs minutes, missing the change
+costs the reason the gate exists.
 
 **Model-comparison results that mean what they say.** Running the golden fixture
-across four models turned up three defects in a row, two fixed and one open:
+across four models turned up three defects in a row, all since fixed:
 
 - `SqliteStore` shared one unguarded connection across the executor threads every
   API route uses — eight concurrent `upsert_document` calls raised three
@@ -141,11 +182,12 @@ across four models turned up three defects in a row, two fixed and one open:
   had the same shape of bug against the same models
   ([#170](https://github.com/vicenteliu/OpsPilot/issues/170)). Both fixed; the
   posture behind them is [ADR-0034](docs/adr/0034-hosted-api-models-are-primary-local-inference-is-auxiliary.md).
-- **Open:** a `ProviderError` silently retries on `extra_models[0]`, so a
-  different model answers and the row keeps the primary's `model_ref`. Naming a
-  model that does not exist currently scores 0.903 and passes
-  ([#175](https://github.com/vicenteliu/OpsPilot/issues/175)). This is the next
-  thing to do — until it is fixed, no comparison table can be trusted.
+- A `ProviderError` silently retried on `extra_models[0]`, so a different model
+  answered and the row kept the primary's `model_ref` — naming a model that does
+  not exist scored 0.903 and passed
+  ([#175](https://github.com/vicenteliu/OpsPilot/issues/175)). Fixed: the swap
+  now writes a `model_fallback` trace event and re-labels the result with the
+  model that actually answered.
 
 **Skill distillation** — shipped
 ([ADR-0026](docs/adr/0026-distillation-target-follows-the-shape-of-the-knowledge.md),
@@ -267,8 +309,14 @@ artifact schema: `intent` is a `const`, so a mutation cannot be expressed.
 Widening it later is a visible, reviewable diff. Playbooks opt in with
 `propose_actions: true`; existing ones are unaffected.
 
-*Not yet:* the UI for previewing and executing — `GET /api/sessions/{id}/actions`,
-`POST .../actions/{ref}/preview` and `POST .../actions/execute` exist.
+The UI for previewing and executing ships with it, over
+`GET /api/sessions/{id}/actions`, `POST .../actions/{ref}/preview` and
+`POST .../actions/execute`.
+
+*Not yet:* **no playbook in this repo opts in**, so on a fresh install the path
+is dark — an escalated Session returns `{"actions": []}`. Outside this file the
+key is named nowhere: not in a playbook, not in ADR-0028, which says only that
+playbooks opt in. The feature is finished; its front door is not open.
 
 **Behaviour gate** — `make test-behaviour`. Five of this product's behaviours are
 produced by a *prompt*, not by code: an injected Memory constraint changing an

examples/sample_data_en/fixtures/README.md

@@ -0,0 +1,25 @@
+# Frozen KB fixtures
+
+Each directory holds a `doc-meta.json` + `chunks.jsonl` pair for one document in
+[`../kb/`](../kb/): the same content, already chunked, with hand-authored
+`document_id` and `chunk_id` values that stay stable across runs.
+
+Load them with:
+
+```bash
+opspilot kb load-dir examples/sample_data_en/fixtures/
+```
+
+`load-dir` bypasses the chunker, the redactor and markitdown, so the ids you get
+are the ids written here. That is the point of a frozen fixture — a test can name
+`chk_f3a40001` and mean it.
+
+**They live outside `../kb/` on purpose.** `README.md` tells a new user to run
+`opspilot ingest examples/sample_data_en/kb/`, and `ingest` is the real pipeline:
+it reads source documents, chunks them, and mints its own ids. Pointing it at a
+directory that also contains these two files made it fail on every `.jsonl` and
+silently ingest every `doc-meta.json` *as a knowledge document* — metadata that
+then came back as an answer. One directory, two commands, neither wrong on its
+own.
+
+So: source documents in `../kb/`, their frozen projections here.

src/opspilot/orchestrator/chat_agent.py

@@ -91,6 +91,11 @@ def _opt_float(value: Any) -> float | None:
     " You have a kb_search tool — call it to ground your answer in the knowledge base "
     "before answering, and base your answer on what it returns."
 )
+_EXHAUSTED_HINT = (
+    " You have used every search you are allowed. Answer now from the results already "
+    "above, and name what is still missing if they are not enough. Do not say you are "
+    "about to look something up — there are no searches left."
+)
 
 
 @dataclass
@@ -381,7 +386,6 @@ def _tools_for_turn() -> list[ToolDef]:
             tools += [t for t in domain_tools if t.name in allowed]
         return tools
 
-    resp: Any = None
     for _ in range(CHAT_MAX_TURNS):
         emit({"type": "status", "message": "Thinking…"})
         resp = provider.chat(
@@ -486,8 +490,22 @@ def _tools_for_turn() -> list[ToolDef]:
 
         return ChatAgentResult(str(resp.content), list(citations.values()), usage)
 
-    # Max turns exhausted — answer with whatever the last turn produced.
-    fallback = (resp.content if resp is not None else "") or (
-        "I couldn't finish searching in time — please narrow your question."
-    )
-    return ChatAgentResult(str(fallback), list(citations.values()), usage)
+    # Every round was a tool call, so the loop ran out before the model wrote an
+    # answer. The last response's content is the preamble it wrote *before*
+    # reaching for a tool — "Let me check the knowledge base…" — and returning
+    # that bills a full search loop and delivers a sentence about intending to
+    # search. Ask once more with no tools: the results are already in
+    # `provider_msgs`, and with nothing left to call, the only move is to answer
+    # from them.
+    # The prompt is rebuilt, not extended: `system_prompt` tells the model to
+    # call kb_search before answering, to call report_conflict *before it
+    # answers*, and which skills it may load — three instructions it can no
+    # longer follow. PROPOSAL_HINT survives because offering a fact to Memory is
+    # prose, not a tool call.
+    emit({"type": "status", "message": "Generating response…"})
+    final_prompt = _SYSTEM_PROMPT_BASE + memory_prefix + PROPOSAL_HINT + _EXHAUSTED_HINT
+    final_msgs = [Message(role="system", content=final_prompt)] + provider_msgs[1:]
+    final = provider.chat(final_msgs, model=model.name, params=sampling)
+    accumulate(final)
+    answer = final.content or "I couldn't finish searching in time — please narrow your question."
+    return ChatAgentResult(str(answer), list(citations.values()), usage)

tests/test_chat_agent.py

@@ -7,7 +7,12 @@
 
 import pytest
 
-from opspilot.orchestrator.chat_agent import CHAT_MAX_TURNS, run_chat_agent
+from opspilot.orchestrator.chat_agent import (
+    _EXHAUSTED_HINT,
+    _TOOL_HINT,
+    CHAT_MAX_TURNS,
+    run_chat_agent,
+)
 from opspilot.providers.types import ChatResponse, ToolCall, Usage
 from opspilot.skills import Skill, SkillRegistry
 
@@ -166,18 +171,61 @@ def test_weak_model_uses_prefetch_no_tools(canned_hits: None) -> None:
 
 def test_max_turns_cap_enforced(canned_hits: None) -> None:
     # Always returns a tool_call → the loop must stop at the cap, not spin forever.
-    provider = FakeProvider(
-        [
-            _resp(
-                "",
-                finish="tool_call",
-                tool_calls=[ToolCall(id="t", name="kb_search", arguments={"query": "q"})],
-            )
-        ]
+    searching = _resp(
+        "Let me check the knowledge base.",
+        finish="tool_call",
+        tool_calls=[ToolCall(id="t", name="kb_search", arguments={"query": "q"})],
     )
+    provider = FakeProvider([searching] * CHAT_MAX_TURNS + [_resp("rotate the cert with certbot")])
     result = run_chat_agent(_state(provider), [{"role": "user", "content": "q"}])
-    assert len(provider.calls) == CHAT_MAX_TURNS
-    assert result.content  # best-effort fallback text, not a crash
+
+    # The cap bounds the *tool* rounds; one final no-tools round turns what was
+    # already retrieved into an answer.
+    assert len(provider.calls) == CHAT_MAX_TURNS + 1
+    assert provider.calls[-1]["tools"] is None
+    # The preamble the model wrote before reaching for a tool is not an answer.
+    assert result.content == "rotate the cert with certbot"
+    # The final round is billed like every other one.
+    assert result.usage["input_tokens"] == 10 * (CHAT_MAX_TURNS + 1)
+
+
+def test_exhausted_loop_never_returns_the_preamble(canned_hits: None) -> None:
+    """A model that answers nothing must not have its "I'll go look" echoed back.
+
+    Regression for the second end-to-end run: six identical `kb_search` rounds
+    billed 578 output tokens and delivered "Let me check the knowledge base for
+    any relevant procedures." with seven citations attached to it.
+    """
+    preamble = "Let me check the knowledge base for any relevant procedures."
+    searching = _resp(
+        preamble,
+        finish="tool_call",
+        tool_calls=[ToolCall(id="t", name="kb_search", arguments={"query": "q"})],
+    )
+    # The final no-tools round returns nothing usable either.
+    provider = FakeProvider([searching] * CHAT_MAX_TURNS + [_resp("")])
+    result = run_chat_agent(_state(provider), [{"role": "user", "content": "q"}])
+    assert preamble not in result.content
+    assert result.content == "I couldn't finish searching in time — please narrow your question."
+
+
+def test_exhausted_round_drops_instructions_for_tools_it_no_longer_has(
+    canned_hits: None,
+) -> None:
+    """The final round has no tools, so it must not be told to call any."""
+    searching = _resp(
+        "",
+        finish="tool_call",
+        tool_calls=[ToolCall(id="t", name="kb_search", arguments={"query": "q"})],
+    )
+    provider = FakeProvider([searching] * CHAT_MAX_TURNS + [_resp("answered")])
+    run_chat_agent(_state(provider), [{"role": "user", "content": "q"}])
+
+    in_loop = provider.calls[0]["messages"][0].content
+    final = provider.calls[-1]["messages"][0].content
+    assert _TOOL_HINT in in_loop  # the tool rounds do offer kb_search
+    assert _TOOL_HINT not in final  # the round with tools=None does not
+    assert _EXHAUSTED_HINT in final
 
 
 def test_unknown_tool_is_reported_not_fatal(canned_hits: None) -> None: