test: close the two live-provider guard gaps from #435 - #440
Merged
Conversation
Gap 1 -- test_glm_fallback_real_llm and test_search_eval_real_llm had no assert_litellm_unpatched() backstop. Rather than four inline asserts the next author has to remember, the e2e conftest now asserts right after it lifts the session patch for a requires_credentials test. Every live test under tests/e2e_tests/ gets the backstop, including ones not written yet, and it also verifies the lift itself took -- anything that patched litellm at another layer, or a future change that quietly makes the suspend a no-op, fails at setup instead of grading canned text. Gap 2 -- test_resumable_extraction_e2e drives a separate server process, so nothing in this interpreter describes it: litellm_is_patched() reports on pytest's own litellm and a local MOCK_LLM_RESPONSE is read by the wrong process, so both pass while the backend serves canned payloads. /healthz now reports the worker's own MOCK_LLM_RESPONSE and the test skips on it. That is also worth reporting for its own sake: a worker answering from the mock is a misconfiguration anywhere real answers are expected, and it is invisible from outside -- callers get plausible text either way. A backend too old to report the field leaves the answer unknown and the run proceeds as before, rather than skipping on a missing key. Verified the backstop fires: with the suspend neutered to a no-op, the live test errors at setup with "litellm.completion is patched" instead of asserting against a profile-extraction payload. Unit tier 4387 passed; e2e tier 47 passed / 87 skipped; ruff + pyright clean.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR updates E2E fixtures to use deterministic extraction cadence, strengthens real-provider credential and LiteLLM checks, and corrects playbook and interaction test lookups. ChangesE2E Reliability and Assertion Alignment
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e_tests/test_resumable_extraction_e2e.py`:
- Around line 173-176: Update the health check exception handling around
_api_request in the live resumable E2E test to skip only connection and timeout
failures, not HTTPError responses from raise_for_status(). Preserve skipping for
an unreachable or timed-out backend while allowing 4xx and 5xx responses to fail
the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ac84cb4-884e-4ff6-8e5f-6469f04262f4
📒 Files selected for processing (4)
reflexio/server/api_endpoints/health_api.pytests/e2e_tests/conftest.pytests/e2e_tests/test_resumable_extraction_e2e.pytests/server/api_endpoints/test_health_api.py
Comment on lines
+173
to
+176
| try: | ||
| health = _api_request("GET", base_url, "/healthz", _live_headers(api_key)) | ||
| except requests.RequestException as exc: | ||
| pytest.skip(f"live resumable E2E backend unreachable at {base_url}: {exc}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'test_resumable_extraction_e2e\.py$' . || true
echo "== file excerpts =="
file="$(fd 'test_resumable_extraction_e2e\.py$' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '140,190p' "$file" | cat -n
sed -n '1,90p' "$file" | cat -n
rg -n "def _api_request|def _live_headers|def _skip_if_backend_is_mocked|raise_for_status|requests\." "$file"
fi
echo "== requests versions/locks =="
fd -a 'requirements.*|pyproject.toml|poetry.lock|uv.lock|Pipfile.lock|setup.cfg|setup.py|Pipfile' . | while read -r f; do
echo "--- $f"
rg -n "requests|python_requires|fastapi|httpx" "$f" || true
doneRepository: ReflexioAI/reflexio
Length of output: 14922
🌐 Web query:
Python requests.RequestException HTTPError subclass raise_for_status documentation
💡 Result:
In the Python Requests library, HTTPError is a subclass of RequestException [1][2][3]. All exceptions explicitly raised by the Requests library inherit from RequestException, which itself is a subclass of IOError [4][2][5]. The raise_for_status method is a function of the Response object that raises an HTTPError if the response contains an unsuccessful HTTP status code [6][4][7]. Specifically, it triggers this exception if the status code is a 4XX (client error) or 5XX (server error) range (400 <= status_code < 600) [8]. If the status code is successful (typically 2XX), the method returns None [4]. Because HTTPError inherits from RequestException, catching RequestException in a try-except block will also catch HTTPError, allowing developers to handle all library-specific exceptions in a single block if desired [4].
Citations:
- 1: https://requests.readthedocs.io/en/stable/_modules/requests/exceptions/
- 2: https://requests.readthedocs.io/en/latest/api/index.html
- 3: https://requests.readthedocs.io/en/latest/_modules/requests/exceptions/
- 4: https://docs.python-requests.org/en/latest/user/quickstart/
- 5: https://github.com/psf/requests/blob/main/src/requests/exceptions.py
- 6: https://docs.python-requests.org/en/stable/api/
- 7: https://docs.python-requests.org/en/latest/api/?highlight=codes
- 8: https://docs.python-requests.org/en/latest/_modules/requests/models/
Let HTTP failures fail the live test.
_api_request calls raise_for_status() before returning, and requests.exceptions.HTTPError is a subclass of requests.RequestException. Catching requests.RequestException here also skips the test for 4xx and 5xx responses, so bad credentials, routing failures, or backend incidents do not expose when the provider assertion fails. Catch only connection and timeout exceptions, or handle intended status codes explicitly.
Proposed fix
- except requests.RequestException as exc:
+ except (
+ requests.exceptions.ConnectionError,
+ requests.exceptions.Timeout,
+ ) as exc:
pytest.skip(f"live resumable E2E backend unreachable at {base_url}: {exc}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| health = _api_request("GET", base_url, "/healthz", _live_headers(api_key)) | |
| except requests.RequestException as exc: | |
| pytest.skip(f"live resumable E2E backend unreachable at {base_url}: {exc}") | |
| try: | |
| health = _api_request("GET", base_url, "/healthz", _live_headers(api_key)) | |
| except ( | |
| requests.exceptions.ConnectionError, | |
| requests.exceptions.Timeout, | |
| ) as exc: | |
| pytest.skip(f"live resumable E2E backend unreachable at {base_url}: {exc}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e_tests/test_resumable_extraction_e2e.py` around lines 173 - 176,
Update the health check exception handling around _api_request in the live
resumable E2E test to skip only connection and timeout failures, not HTTPError
responses from raise_for_status(). Preserve skipping for an unreachable or
timed-out backend while allowing 4xx and 5xx responses to fail the test.
Running the e2e tier with RUN_LOW_PRIORITY=1 gave 8 failed / 101 passed /
25 skipped. None of the failures were product bugs; all were test-side rot
that had accumulated behind the opt-in gate. Four distinct causes:
Stride gate. DEFAULT_STRIDE_SIZE was raised 5 -> 8 ("update stride size to 8
to save cost"). The e2e fixtures inherited it, so every test publishing fewer
than 8 interactions stopped extracting entirely and asserted against zero
profiles/playbooks. Pin an explicit _E2E_STRIDE_SIZE on the fixtures so a
production cost knob can no longer decide whether extraction runs at all.
Playbook name drift. Raw playbooks are stored under
SINGLETON_USER_PLAYBOOK_NAME ("playbook"), not the config's extractor_name.
Three call sites queried "test_playbook" and matched nothing. In the
concurrent test that read as "the R2 bug is back"; in
test_rerun_operations_consistency the "playbooks unchanged" assertion was
comparing 0 == 0 and could not fail. It now compares 1 == 1.
Orphaned query string. test_dict_input_handling_end_to_end searched for
"Sarah" long after the fixture moved to the Priya scenario — the only such
reference left in the suite.
Missing credential gates. test_search_eval_real_llm pins claude-haiku-4-5
and a claude-sonnet-4-6 judge but gated on nothing, so without the key it
died on AuthenticationError instead of skipping. It now gates on
ANTHROPIC_API_KEY. test_profile_dedup_resolves_contradiction asserts the
dedup model resolves contradictions, which the echoing mock can never
satisfy; it now carries requires_credentials plus a real-provider gate and
runs against whichever generation provider has a key.
Tier after: 105 passed, 27 skipped, 0 failed (plus two tests needing caplog,
both passing). The two dedup cases now execute against MiniMax.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e_tests/conftest.py (1)
146-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the downstream stride comment.
At Line 146,
reflexio_instancenow uses_E2E_STRIDE_SIZE, which is1. The comment intests/e2e_tests/test_complete_workflows.pyat Lines 358-359 still saysstride=5. Update that comment to match the fixture configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e_tests/conftest.py` at line 146, Update the stride comment in test_complete_workflows.py to say stride=1, matching the _E2E_STRIDE_SIZE value used by the reflexio_instance fixture; do not change the fixture or workflow behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/e2e_tests/conftest.py`:
- Line 146: Update the stride comment in test_complete_workflows.py to say
stride=1, matching the _E2E_STRIDE_SIZE value used by the reflexio_instance
fixture; do not change the fixture or workflow behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c0294e2-fe08-46b2-a70f-f859acb23937
📒 Files selected for processing (6)
tests/e2e_tests/conftest.pytests/e2e_tests/test_complete_workflows.pytests/e2e_tests/test_concurrent_playbook_extraction.pytests/e2e_tests/test_interaction_workflows.pytests/e2e_tests/test_profile_workflows.pytests/e2e_tests/test_search_eval_real_llm.py
The comment claimed stride=5 while DEFAULT_STRIDE_SIZE was 8 -- stale before this branch touched it. Point at _E2E_STRIDE_SIZE instead so it cannot drift again.
Removes the production API surface this branch added, and fixes the
review-loop findings on the rest.
Drop `mock_llm_response` from GET /healthz, its two unit tests, and the
`_skip_if_backend_is_mocked` helper that consumed it. The field widened a
production endpoint to serve one opt-in local test, and /healthz answers
unauthenticated (reproduced: create_app(require_auth=True) returns 200 with
no Authorization header). The documented ALB never routes /healthz, so the
"operators can see this" rationale was unreachable anyway. The helper it fed
was itself a check that could not fail: `_api_request` calls
raise_for_status(), and HTTPError/JSONDecodeError are RequestException
subclasses, so a 401 from a stale key became skip("backend unreachable").
The live E2E keeps its RUN_LIVE_RESUMABLE_E2E opt-in and now fails loudly
against a mocked backend instead of skipping.
Close the guard gaps this branch fixed by line rather than by class:
- test_knowledge_gap_real_llm.py had `requires_credentials` and no skipif.
That marker does not skip on its own -- it deselects and lifts the mock --
so a keyless run called the provider with the pinned placeholder and died
on AuthenticationError, the exact failure #435 was about.
- test_glm_fallback_real_llm.py gated on `os.environ.get("ZAI_API_KEY")`,
which accepts the placeholder the credential floor pins; now
real_provider_key.
- stride_size was pinned on 3 of 10 e2e fixtures; now all 10.
Also: restrict the dedup gate to providers its assertions were validated
against, and add a non-vacuity floor so the rerun test's "playbooks
unchanged" check cannot return to comparing 0 == 0.
Verified with placeholder keys for every provider: 44 skipped, 0 failed,
0 errors -- every live-provider test now skips rather than erroring.
Two review-loop findings against the previous commit.
Restore the local MOCK_LLM_RESPONSE check in _load_live_e2e_settings. main
had this guard; the first commit on this branch replaced it with the
/healthz probe, and removing that probe left the live E2E with NO mock guard
at all -- strictly worse than main. It is a partial signal (it cannot
describe a remote backend) but it catches the common case where the backend
runs from the same shell, and it is the only one available without widening
a production endpoint. The comment now says so rather than overclaiming.
assert_litellm_unpatched() asked only the patcher, so it could not fail in
the mode it advertises: 12 service sites branch on MOCK_LLM_RESPONSE and
return canned payloads without calling litellm at all. Reproduced --
litellm_is_patched() False, the assert passing, and the extractor's mock
branch firing. It now checks both mechanisms. Two tests cover it, and the
positive one was mutation-verified: with the new branch surgically removed
it fails, with it restored it passes; the paired inverse test rules out a
guard that passes by always raising.
Also correct the _DEDUP_CAPABLE comment, which called MiniMax "validated"
while the docstring records it failing roughly half the time on
diet_reversal. It is exercised, not validated.
Known-failing under RUN_LOW_PRIORITY=1, both real-LLM nondeterminism and
neither caused by this branch:
- test_profile_dedup_resolves_contradiction[diet_reversal] (~50%)
- test_minimax_to_glm_fallback -- fails identically with these files
reverted to origin/main, so it is pre-existing; the fallback mechanism
itself works (event=llm_fallback_used served_model=zai/glm-5.2), GLM
just returns no playbook row.
Both are skip_low_priority + requires_credentials, so neither runs in CI.
Both were extraction being suppressed or under-evidenced, not model quality.
Neither was introduced by this branch; both failed on origin/main too.
test_minimax_to_glm_fallback. GLM sometimes emitted a single vague playbook
candidate that the quality reviewer then rejected -- "Extracted 1 playbook
entries", "accepted=0 rejected=1 reason_codes=generic:1" -- versus 3 extracted
and 3 accepted on a good run. The reviewer was correct; the defect was the
input, which states its preferences indirectly. Append four explicit
first-person corrective turns so the reviewer has direct user evidence to
ground a playbook on. The reviewer is untouched and the assertion is
unchanged. 6/6 passes across 3 runs, from roughly 4 failures in 5.
test_profile_dedup_resolves_contradiction. Two failure modes, one cause: the
should_run LLM gate nondeterministically returned False and skipped
extraction ("Pre-extraction check returned False for profile_generation ...
skipping" x3 on a failing run, 0 on a passing one). When it skipped batch 2,
the stale batch-1 profile survived a dedup that never ran -- which read as
the model failing to resolve a contradiction it was never asked about. Only
2 of 10 e2e fixtures pinned skip_should_run_check; pin it on all of them, the
same gate-suppression class as the stride pin. No e2e test covers the gate
itself, so no coverage is lost. 10/10 passes across 5 runs with
should_run_skips=0 every run.
Correct the docstring and _DEDUP_CAPABLE comment, which blamed MiniMax's
dedup judgement for what was a skipped extraction.
Tier: 107 passed, 27 skipped, 0 failed (was 8 failed / 101 passed / 25
skipped at the start of this work).
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The two follow-ups recorded when #435 was closed.
Gap 1 — two live tests had no backstop
test_glm_fallback_real_llm.pyandtest_search_eval_real_llm.pycarried noassert_litellm_unpatched().Fixed one level up instead of four inline asserts: the e2e conftest now asserts immediately after it lifts the session patch for a
requires_credentialstest. Every live test undertests/e2e_tests/gets the backstop — including ones not written yet — and it additionally verifies the lift itself took. Anything that patched litellm at another layer, or a future change that quietly makes the suspend a no-op, now fails at fixture setup instead of grading canned text.Verified it fires. With
unpatched_litellm's suspend neutered to a no-op:instead of the previous silent
assert '4' in '```json\n{"add": [{"content": "prefers dark mode"....Gap 2 — an out-of-process test needs an out-of-process signal
test_resumable_extraction_e2edrives a separate server ("requires live provider credentials in that backend process"). Nothing in the pytest interpreter describes it:litellm_is_patched()reports on pytest's own litellm, and a localMOCK_LLM_RESPONSEis read by the wrong process. Both pass while the backend serves canned payloads./healthznow reports the worker's ownMOCK_LLM_RESPONSE, and the test skips on it.That field earns its place beyond this test: a worker answering from the canned mock is a misconfiguration anywhere real answers are expected, and it is not otherwise observable from outside the process — a caller gets plausible-looking text either way.
/healthzis already the diagnostics endpoint and is installed unconditionally (api.py:680).A backend too old to report the field leaves the answer unknown; the run then proceeds exactly as it does today rather than skipping on a missing key.
Note on scope
This adds a field to a production response (
/healthz), which is more than a test-only change. It is additive, reads one env var, and/healthzappears in no public docs ordocs_for_coding_agent— only historical plan files. Happy to drop it and leave gap 2 documented-but-unfixed if you'd rather not grow that payload; there is no purely test-side way to observe another process's mock state.Verification
4387 passed, 9 skipped47 passed, 87 skipped-m e2elive testAuthenticationErroron a deliberately invalid key)Two new
/healthztests cover the field, including that only an explicit"true"counts.Summary by CodeRabbit