Fix auth bugs, improve error handling, and add integration tests#10
Conversation
Bug fixes: - Fix 401 retry silently dropping responses when refresh fails. The generator now ends gracefully and the caller receives the original 401 instead of no response at all. - Fix incorrect return type on OciSessionAuth._load_private_key (str -> Any). The OCI SDK returns a cryptography key object, not a string. Improvements: - Add OciAuthRefreshError exception with cause tracking and elapsed time since last successful refresh, for better diagnostics. - Track refresh failures via _last_refresh_error attribute (cleared on success, set on failure) so callers can inspect auth health. - Reduce logging noise: init and routine refresh checks moved from INFO to DEBUG. Only actual refresh completions stay at INFO. Scheduled refresh failures log at WARNING (not silent exception). 401 retry failures log at ERROR with actionable context. Testing: - Expand unit tests from 9 to 20 covering: signing path with header stripping, POST body signing, 401 retry behavior, non-401 passthrough, refresh failure tracking, error clearing on success, OciAuthRefreshError formatting, session auth missing key_file, config reload on refresh. - Add integration test infrastructure: conftest.py with env-based config loading, session-scoped fixtures, and requires_oci skip marker. - Add tests/.env.example template for integration test configuration. - Add 4 live integration tests: sync/async Responses API, raw httpx signing, and header stripping verification. Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
- Fix SIM105: replace try/except StopIteration/pass with contextlib.suppress(StopIteration) - Fix B011: replace assert False with pytest.raises - Apply isort + ruff format to all changed files - Ensure isort → ruff format → ruff check --fix produces stable output Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
- Remove dependency on model echoing exact text -- assertions now verify non-empty responses and HTTP 200 status instead of specific content, since model output is non-deterministic. - Expand .env.example with prerequisites: how to create a GenAI Project, which models are known to work/not work on /openai/v1, and auth setup instructions. Signed-off-by: Federico Kamelhar <federico.kamelhar@oracle.com>
|
@shenoyvvarun Would appreciate your review on this. Fixes a bug where the 401 retry path silently drops responses, improves logging levels, adds OciAuthRefreshError for refresh diagnostics, and expands test coverage from 9 to 20 unit tests plus 4 integration tests. |
|
Friendly ping — this PR has been open for ~11 days without a review. Happy to address feedback or rebase further if needed. Let me know if there's anything I can clarify. Thanks! |
| OciAuthSigner: TypeAlias = oci.signer.AbstractBaseSigner | ||
|
|
||
|
|
||
| class OciAuthRefreshError(Exception): |
There was a problem hiding this comment.
Done — removed the class, its export from __init__.py, and the two tests that constructed it.
You're right that it's dead code: nothing in the production path actually raises it. The two failure sites both swallow + log:
- scheduled refresh in
_refresh_if_neededstores the raw exc onself._last_refresh_errorand keeps serving the previous signer (graceful degradation) - 401-retry refresh logs and lets the original 401 bubble up to the caller
So the class was unreachable. I'd originally added it thinking we might want a structured error (with last_success elapsed time) for observability/metrics, but since we never raise it, it just bloats the public API. Dropping it.
There was a problem hiding this comment.
Looks like it still exists... Maybe missed commit push?
There was a problem hiding this comment.
You're right — sorry, the deletion commit landed on my framework-helpers branch by mistake. Just cherry-picked it onto fix/auth-improvements (a3523ce) and pushed. Should be gone now.
Integration test results (post-fix)Ran the full integration suite ( Coverage:
Plus 33/33 unit tests pass and ruff/isort are clean. |
The class was never raised — both refresh failure paths swallow the raw exception and store it on _last_refresh_error. Drop the class, its public re-export, and the two tests that constructed it directly.
shenoyvvarun
left a comment
There was a problem hiding this comment.
LGTM. Thanks for making the change.
Summary
OciSessionAuth._load_private_key(str→Any). The OCI SDK returns a cryptography key object, not a string.OciAuthRefreshErrorexception with cause tracking and elapsed time since last successful refresh._last_refresh_errorattribute for callers to inspect auth health.INFOtoDEBUG. Scheduled refresh failures log atWARNING. 401 retry failures log atERRORwith actionable context.conftest.pywith env-based config,tests/.env.exampletemplate, andrequires_ociskip marker.Test plan
pytest tests/test_auth.py)pytest tests/integration/test_auth_live.py)OCI_GENAI_*env varsOciAuthRefreshError)How to validate
Fixes #12