Skip to content

OfficeQA Keenable search: redact the key, enforce the round deadline, tighten tests - #381

Open
Ilya Bogin (ilya-bogin-keenable) wants to merge 1 commit into
microsoft:mainfrom
keenableai:fix/keenable-search-followups
Open

Ilya Bogin (ilya-bogin-keenable) wants to merge 1 commit into
microsoft:mainfrom
keenableai:fix/keenable-search-followups

Conversation

@ilya-bogin-keenable

Copy link
Copy Markdown
Contributor

Follow-ups from the #368 review, one item per point.

  1. Key in error text. KEENABLE_API_KEY is replaced with [redacted] in the HTTP error body, before the 1000-character cut, so a key straddling the cut can't leak a prefix either. Tests cover a key echoed in a 401 body (checked in fail_reason and conversation.json) and one at the cut.
  2. Round deadline. time.monotonic() > deadline is checked again after each response, so a slow last query now fails the round too, as the docs say. Test: a single query whose response lands past the deadline.
  3. Tests.
    • test_fresh_config_never_calls_keenable now has the model ask for search, then asserts the mock transport saw no request.
    • New tests cover a 302 (not followed, one request), a non-chat backend (refused before any request), and a non-JSON body (fails instead of returning empty results).
    • I mutated the code to check that each one fails on the regression you listed: follow_redirects=True, no backend check, invalid JSON read as [], no redaction, no post-response deadline check, and auto selecting Keenable (the old fresh-config test passed with that one).
  4. Minor.
    • The config table now says search_max_num_results is per query for Keenable: 16 results, about 97k characters, per round with the defaults.
    • The data-boundary note says a query can repeat text from the document context or the skill as well.
    • A missing or non-string title falls back to the URL.

tests/test_officeqa_keenable_search.py: 42 passed. The full suite passes on 3.10 and 3.12, apart from failures that also happen on an unmodified ccfdcdf in my environment: test_backend_pi::test_default_model_from_env, two test_sleep_adopt_skill_subset tampered-manifest tests, and, on 3.10 only, test_officeqa_parallel_search::test_whole_mcp_request_has_a_deadline.

- Redact KEENABLE_API_KEY from the HTTP error text before it is cut and
  reaches fail_reason, results.jsonl and the reflection prompt.
- Check the round deadline again after each response: httpx applies the
  timeout per phase, so a slow last response could end past
  search_timeout_seconds unchecked.
- Fall back to the URL for a missing or non-string result title.
- Tests: a 302 is not followed, a non-chat backend is refused before any
  request, a non-JSON body fails, a key echoed in a 401 body (and one
  straddling the 1000-character cut) is redacted, a response past the
  deadline fails the round. test_fresh_config_never_calls_keenable now has
  the model ask for search and asserts the transport saw no request; before,
  the model answered on the first turn and an AssertionError inside
  process_one was swallowed, so it could not fail.
- Docs: Keenable's search_max_num_results is per query (16 results per
  round with the defaults), and a generated query can repeat text from the
  document context or the skill, not only the question.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant