diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index de159fe..21e7e7b 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/Makefile b/Makefile index 7be8b2f..312b8ac 100644 --- a/Makefile +++ b/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`). diff --git a/ROADMAP.md b/ROADMAP.md index 055fdb9..702ad7b 100644 --- a/ROADMAP.md +++ b/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:` 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 diff --git a/examples/sample_data_en/fixtures/README.md b/examples/sample_data_en/fixtures/README.md new file mode 100644 index 0000000..d0936e5 --- /dev/null +++ b/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. diff --git a/examples/sample_data_en/kb/aws_iam_errors/chunks.jsonl b/examples/sample_data_en/fixtures/aws_iam_errors/chunks.jsonl similarity index 100% rename from examples/sample_data_en/kb/aws_iam_errors/chunks.jsonl rename to examples/sample_data_en/fixtures/aws_iam_errors/chunks.jsonl diff --git a/examples/sample_data_en/kb/aws_iam_errors/doc-meta.json b/examples/sample_data_en/fixtures/aws_iam_errors/doc-meta.json similarity index 100% rename from examples/sample_data_en/kb/aws_iam_errors/doc-meta.json rename to examples/sample_data_en/fixtures/aws_iam_errors/doc-meta.json diff --git a/examples/sample_data_en/kb/db_connection_pool/chunks.jsonl b/examples/sample_data_en/fixtures/db_connection_pool/chunks.jsonl similarity index 100% rename from examples/sample_data_en/kb/db_connection_pool/chunks.jsonl rename to examples/sample_data_en/fixtures/db_connection_pool/chunks.jsonl diff --git a/examples/sample_data_en/kb/db_connection_pool/doc-meta.json b/examples/sample_data_en/fixtures/db_connection_pool/doc-meta.json similarity index 100% rename from examples/sample_data_en/kb/db_connection_pool/doc-meta.json rename to examples/sample_data_en/fixtures/db_connection_pool/doc-meta.json diff --git a/examples/sample_data_en/kb/k8s_pod_crash/chunks.jsonl b/examples/sample_data_en/fixtures/k8s_pod_crash/chunks.jsonl similarity index 100% rename from examples/sample_data_en/kb/k8s_pod_crash/chunks.jsonl rename to examples/sample_data_en/fixtures/k8s_pod_crash/chunks.jsonl diff --git a/examples/sample_data_en/kb/k8s_pod_crash/doc-meta.json b/examples/sample_data_en/fixtures/k8s_pod_crash/doc-meta.json similarity index 100% rename from examples/sample_data_en/kb/k8s_pod_crash/doc-meta.json rename to examples/sample_data_en/fixtures/k8s_pod_crash/doc-meta.json diff --git a/examples/sample_data_en/kb/oncall_escalation/chunks.jsonl b/examples/sample_data_en/fixtures/oncall_escalation/chunks.jsonl similarity index 100% rename from examples/sample_data_en/kb/oncall_escalation/chunks.jsonl rename to examples/sample_data_en/fixtures/oncall_escalation/chunks.jsonl diff --git a/examples/sample_data_en/kb/oncall_escalation/doc-meta.json b/examples/sample_data_en/fixtures/oncall_escalation/doc-meta.json similarity index 100% rename from examples/sample_data_en/kb/oncall_escalation/doc-meta.json rename to examples/sample_data_en/fixtures/oncall_escalation/doc-meta.json diff --git a/examples/sample_data_en/kb/tls_cert_mgmt/chunks.jsonl b/examples/sample_data_en/fixtures/tls_cert_mgmt/chunks.jsonl similarity index 100% rename from examples/sample_data_en/kb/tls_cert_mgmt/chunks.jsonl rename to examples/sample_data_en/fixtures/tls_cert_mgmt/chunks.jsonl diff --git a/examples/sample_data_en/kb/tls_cert_mgmt/doc-meta.json b/examples/sample_data_en/fixtures/tls_cert_mgmt/doc-meta.json similarity index 100% rename from examples/sample_data_en/kb/tls_cert_mgmt/doc-meta.json rename to examples/sample_data_en/fixtures/tls_cert_mgmt/doc-meta.json diff --git a/src/opspilot/orchestrator/chat_agent.py b/src/opspilot/orchestrator/chat_agent.py index 320579d..3dcc0e5 100644 --- a/src/opspilot/orchestrator/chat_agent.py +++ b/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) diff --git a/tests/test_chat_agent.py b/tests/test_chat_agent.py index c79404a..2d4e6fd 100644 --- a/tests/test_chat_agent.py +++ b/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: