diff --git a/CHANGELOG.md b/CHANGELOG.md index 04cb3d90..20c578ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,9 @@ All notable changes to SkillOpt are documented here. This project adheres to key, credential or account is bundled, and service errors fail the rollout with no fallback. It uses the core `httpx` dependency, so it needs no extra. Its scores are not directly comparable to the paper's Google-backed - search setting. + search setting. An API key echoed in an error body is redacted before the + text reaches `fail_reason`, and the round deadline is checked again after + each response. - **SkillOpt-Sleep paired A/B evalkit** (`python -m skillopt_sleep.evalkit`): McNemar plus percentile-bootstrap CIs on a fixed task manifest, with task-cluster multi-seed inference and seeded null calibration for exact-test diff --git a/docs/reference/config.md b/docs/reference/config.md index c5afa22a..a3c723b3 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -224,7 +224,7 @@ corpus and makes no network search call. | `env.search_mode: parallel` | Opt in to Parallel's hosted Search MCP. Requires `openai_chat` or `qwen_chat` and the `parallel` extra. | | `env.search_mode: keenable` | Opt in to Keenable's search API. Requires `openai_chat` or `qwen_chat`; no extra and no key needed. | | `env.search_timeout_seconds` | Whole MCP discovery/search deadline in seconds (default `20`); for `keenable`, the deadline for all queries of one round. | -| `env.search_max_num_results` | Maximum number of Parallel evidence results per round, or Keenable results per query (default `4`). | +| `env.search_max_num_results` | Maximum number of Parallel evidence results per round, or Keenable results per query (default `4`). For Keenable a round can therefore carry up to `max_queries_per_turn × search_max_num_results` results: 16 with the defaults, up to about 97k characters of evidence. | | `env.max_queries_per_turn` / `env.max_tool_turns` | Bound model-requested queries and model rounds. | #### Parallel Search MCP (opt-in) @@ -276,7 +276,9 @@ query from the model's generated `` to Keenable, one HTTPS - only if you set `KEENABLE_API_KEY`: that key, in an `X-API-Key` header. SkillOpt does not send the task question, the OfficeQA corpus or any other -credential, although a generated query can repeat words from the question. +credential as such. A generated query is model output, though, so it can +repeat text from the question, from the provided document context or from the +skill. Without `KEENABLE_API_KEY` the requests go to the keyless endpoint `https://api.keenable.ai/v1/search/public`, which is limited to 10 requests per second and 1,000 requests per hour per IP address. With the key they go to diff --git a/skillopt/envs/officeqa/keenable_search.py b/skillopt/envs/officeqa/keenable_search.py index b62e94fb..d21e55c6 100644 --- a/skillopt/envs/officeqa/keenable_search.py +++ b/skillopt/envs/officeqa/keenable_search.py @@ -35,7 +35,10 @@ def _render(index: int, query: str, results: list, max_num_results: int) -> str: raise RuntimeError("Keenable search returned an invalid result URL") if not isinstance(snippet, str): raise RuntimeError("Keenable search returned an invalid snippet") - blocks.append(f"[{len(blocks) + 1}] {item.get('title') or url}\nURL: {url}\n{snippet.strip()[:6000]}") + title = item.get("title") + if not isinstance(title, str) or not title.strip(): + title = url + blocks.append(f"[{len(blocks) + 1}] {title.strip()}\nURL: {url}\n{snippet.strip()[:6000]}") body = "\n\n".join(blocks) if blocks else "[no search results]" return f"## Query {index}\nQuery: {query}\n\n{body}" @@ -68,8 +71,18 @@ def keenable_search(queries: list[str], *, max_num_results: int = 4, timeout: fl ) except httpx.TimeoutException as exc: raise TimeoutError(f"Keenable search exceeded its {timeout}s deadline") from exc + # httpx applies `timeout` per phase, so a slow response can still end + # past the round deadline; check it again once the response is in. + if time.monotonic() > deadline: + raise TimeoutError(f"Keenable search exceeded its {timeout}s deadline") if response.status_code != 200: - raise RuntimeError(f"Keenable search HTTP {response.status_code}: {response.text[:1000]}") + # The error text reaches fail_reason, results.jsonl and the + # reflection prompt, so never let an echoed key travel with it. + # Redact before cutting, so a key straddling the cut can't leak a prefix. + detail = response.text + if api_key: + detail = detail.replace(api_key, "[redacted]") + raise RuntimeError(f"Keenable search HTTP {response.status_code}: {detail[:1000]}") try: payload = response.json() except ValueError as exc: diff --git a/tests/test_officeqa_keenable_search.py b/tests/test_officeqa_keenable_search.py index 15592fa4..45e4d8cb 100644 --- a/tests/test_officeqa_keenable_search.py +++ b/tests/test_officeqa_keenable_search.py @@ -27,11 +27,13 @@ def isolated_search_config(monkeypatch): def transport(monkeypatch): """Record the HTTP requests the client sends and answer them from `behavior`.""" requests = [] - behavior = {"payload": PAYLOAD, "status": 200, "timeout": False} + behavior = {"payload": PAYLOAD, "status": 200, "timeout": False, "response": None} original_client = httpx.Client def handler(request): requests.append((request, json.loads(request.content))) + if behavior["response"] is not None: + return behavior["response"] if behavior["timeout"]: raise httpx.ReadTimeout("read timed out", request=request) if behavior["status"] != 200: @@ -76,13 +78,17 @@ def test_keenable_requires_explicit_selection(mode): @pytest.mark.parametrize("mode", ["auto", "offline"]) -def test_fresh_config_never_calls_keenable(tmp_path, monkeypatch, mode): +def test_fresh_config_never_calls_keenable(tmp_path, monkeypatch, transport, mode): docs = tmp_path / "docs" docs.mkdir() - monkeypatch.setattr(rollout, "chat_target_messages", lambda **kwargs: (SimpleNamespace(content="1776"), {})) - with mock.patch.object(rollout, "keenable_search", side_effect=AssertionError("keenable not selected")): - row = rollout.process_one(ITEM, str(tmp_path / "out"), "", search_mode=mode, use_local_tools=False, data_dirs=[str(docs)]) - assert row["predicted_answer"] == "1776" + # The model asks for search; outside explicit keenable mode nothing may reach Keenable. + responses = iter(['["declaration treasury history"]']) + monkeypatch.setattr( + rollout, "chat_target_messages", + lambda **kwargs: (SimpleNamespace(content=next(responses, "1776")), {}), + ) + rollout.process_one(ITEM, str(tmp_path / "out"), "", search_mode=mode, use_local_tools=False, data_dirs=[str(docs)]) + assert transport[0] == [] # ── Rollout through the keyless endpoint ───────────────────────────────────── @@ -201,3 +207,63 @@ def test_invalid_arguments_are_refused_before_any_request(transport, queries, kw with pytest.raises(ValueError): keenable.keenable_search(queries, **kwargs) assert not transport[0] + + +# ── Follow-ups from the #368 review ────────────────────────────────────────── + +def test_a_redirect_is_not_followed(monkeypatch, transport): + # Following it would forward X-API-Key to another origin. + monkeypatch.setenv("KEENABLE_API_KEY", "kn-test-key") + transport[1]["response"] = httpx.Response(302, headers={"Location": "https://elsewhere.example/steal"}) + with pytest.raises(RuntimeError, match="Keenable search HTTP 302"): + keenable.keenable_search(["treasury"]) + assert [str(request.url) for request, _ in transport[0]] == [keenable.KEYED_URL] + + +def test_a_non_json_body_fails_instead_of_returning_no_results(transport): + transport[1]["response"] = httpx.Response(200, text="maintenance") + with pytest.raises(RuntimeError, match="invalid JSON"): + keenable.keenable_search(["treasury"]) + + +def test_a_key_echoed_in_an_error_body_is_redacted(tmp_path, monkeypatch, transport): + monkeypatch.setenv("KEENABLE_API_KEY", "kn-secret-key") + transport[1]["response"] = httpx.Response(401, text="invalid key kn-secret-key for this org") + scripted_model(monkeypatch) + row = rollout.process_one(ITEM, str(tmp_path), "", search_mode="keenable", max_tool_turns=2) + assert "Keenable search HTTP 401" in row["fail_reason"] + assert "[redacted]" in row["fail_reason"] + assert "kn-secret-key" not in row["fail_reason"] + assert "kn-secret-key" not in (tmp_path / "predictions" / ITEM["id"] / "conversation.json").read_text() + + +def test_a_key_straddling_the_error_cut_leaks_no_prefix(monkeypatch, transport): + monkeypatch.setenv("KEENABLE_API_KEY", "kn-secret-key") + transport[1]["response"] = httpx.Response(401, text="x" * 995 + "kn-secret-key") + with pytest.raises(RuntimeError) as excinfo: + keenable.keenable_search(["treasury"]) + assert "kn-se" not in str(excinfo.value) + + +@pytest.mark.parametrize("backend", ["claude_exec", "codex_exec", "azure_openai_responses"]) +def test_keenable_refuses_a_non_chat_backend_before_any_request(tmp_path, monkeypatch, transport, backend): + monkeypatch.setattr(rollout, "get_target_backend", lambda: backend) + scripted_model(monkeypatch) + row = rollout.process_one(ITEM, str(tmp_path), "", search_mode="keenable", max_tool_turns=2) + assert "only supported with target_backend='openai_chat' or 'qwen_chat'" in row["fail_reason"] + assert transport[0] == [] + + +def test_a_response_that_ends_past_the_deadline_fails_the_round(monkeypatch, transport): + # One query: checked before the request at t=0, the response lands at t=25. + clock = iter([0.0, 0.0, 25.0]) + monkeypatch.setattr(keenable.time, "monotonic", lambda: next(clock)) + with pytest.raises(TimeoutError): + keenable.keenable_search(["treasury"], timeout=20) + assert len(transport[0]) == 1 + + +@pytest.mark.parametrize("title", [None, "", " ", 42, ["Treasury"]]) +def test_a_missing_or_non_string_title_falls_back_to_the_url(transport, title): + transport[1]["payload"] = {"results": [{"url": "https://example.org/t", "title": title, "snippet": "s"}]} + assert "[1] https://example.org/t\nURL: https://example.org/t" in keenable.keenable_search(["treasury"])