-
Notifications
You must be signed in to change notification settings - Fork 3
fix: load API keys from database at startup + setup wizard UX improvements #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
0pfleet
wants to merge
7
commits into
hyperb1iss:main
Choose a base branch
from
0pfleet:fix/api-key-loading-and-setup-ux
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b83e97e
fix(api): load API keys from database at server/worker startup
0pfleet 99af0ae
fix(api): hot-reload API keys when updated via webapp
0pfleet e468e10
fix(web): persist setup wizard step across tab switches
0pfleet f2835da
fix(web): improve API key setup UX with clear save feedback
0pfleet 8f507fa
test: add regression tests for API key loading and setup wizard
0pfleet db79853
refactor: address CodeRabbit review feedback
0pfleet 6bc1c66
fix: remove unused os import in worker startup
0pfleet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| """Tests for API key loading from database at startup. | ||
|
|
||
| These tests verify the fix for the issue where API keys configured via the webapp | ||
| were not available to GraphClient at startup because it reads from environment | ||
| variables at import time. | ||
| """ | ||
|
|
||
| import os | ||
| from unittest.mock import AsyncMock, MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| class TestApiKeyLoadingAtStartup: | ||
| """Tests for API key loading during server/worker startup.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_api_key_loaded_from_db_when_env_not_set(self, monkeypatch) -> None: | ||
| """Verify API keys are loaded from DB into os.environ when env vars are not set.""" | ||
| # Clear any existing env vars | ||
| monkeypatch.delenv("OPENAI_API_KEY", raising=False) | ||
| monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) | ||
|
|
||
| # Mock the settings service | ||
| mock_settings_service = AsyncMock() | ||
| mock_settings_service.get_openai_key = AsyncMock(return_value="sk-test-openai-key") | ||
| mock_settings_service.get_anthropic_key = AsyncMock(return_value="sk-ant-test-key") | ||
|
|
||
| with patch( | ||
| "sibyl.services.settings.get_settings_service", return_value=mock_settings_service | ||
| ): | ||
| # Simulate the key loading logic from main.py | ||
| if not os.environ.get("OPENAI_API_KEY"): | ||
| openai_key = await mock_settings_service.get_openai_key() | ||
| if openai_key: | ||
| os.environ["OPENAI_API_KEY"] = openai_key | ||
|
|
||
| if not os.environ.get("ANTHROPIC_API_KEY"): | ||
| anthropic_key = await mock_settings_service.get_anthropic_key() | ||
| if anthropic_key: | ||
| os.environ["ANTHROPIC_API_KEY"] = anthropic_key | ||
|
|
||
| assert os.environ.get("OPENAI_API_KEY") == "sk-test-openai-key" | ||
| assert os.environ.get("ANTHROPIC_API_KEY") == "sk-ant-test-key" | ||
|
|
||
| # Cleanup | ||
| monkeypatch.delenv("OPENAI_API_KEY", raising=False) | ||
| monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) | ||
0pfleet marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_env_var_takes_precedence_over_db(self, monkeypatch) -> None: | ||
| """Verify existing env vars are not overwritten by DB values.""" | ||
| # Set existing env vars | ||
| monkeypatch.setenv("OPENAI_API_KEY", "sk-existing-env-key") | ||
| monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-existing-env-key") | ||
|
|
||
| # Mock the settings service with different values | ||
| mock_settings_service = AsyncMock() | ||
| mock_settings_service.get_openai_key = AsyncMock(return_value="sk-db-openai-key") | ||
| mock_settings_service.get_anthropic_key = AsyncMock(return_value="sk-ant-db-key") | ||
|
|
||
| with patch( | ||
| "sibyl.services.settings.get_settings_service", return_value=mock_settings_service | ||
| ): | ||
| # Simulate the key loading logic - should NOT overwrite | ||
| if not os.environ.get("OPENAI_API_KEY"): | ||
| openai_key = await mock_settings_service.get_openai_key() | ||
| if openai_key: | ||
| os.environ["OPENAI_API_KEY"] = openai_key | ||
|
|
||
| if not os.environ.get("ANTHROPIC_API_KEY"): | ||
| anthropic_key = await mock_settings_service.get_anthropic_key() | ||
| if anthropic_key: | ||
| os.environ["ANTHROPIC_API_KEY"] = anthropic_key | ||
|
|
||
| # Verify env vars were NOT overwritten | ||
| assert os.environ.get("OPENAI_API_KEY") == "sk-existing-env-key" | ||
| assert os.environ.get("ANTHROPIC_API_KEY") == "sk-ant-existing-env-key" | ||
|
|
||
| # Verify DB was not even queried (optimization) | ||
| mock_settings_service.get_openai_key.assert_not_called() | ||
| mock_settings_service.get_anthropic_key.assert_not_called() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_api_key_loading_failure_does_not_crash(self, monkeypatch) -> None: | ||
| """Ensure startup continues gracefully if DB query fails.""" | ||
| monkeypatch.delenv("OPENAI_API_KEY", raising=False) | ||
| monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) | ||
|
|
||
| # Mock settings service that raises an exception | ||
| mock_settings_service = AsyncMock() | ||
| mock_settings_service.get_openai_key = AsyncMock( | ||
| side_effect=Exception("Database connection failed") | ||
| ) | ||
|
|
||
| error_logged = False | ||
|
|
||
| with patch( | ||
| "sibyl.services.settings.get_settings_service", return_value=mock_settings_service | ||
| ): | ||
| # Simulate the try/except from main.py | ||
| try: | ||
| if not os.environ.get("OPENAI_API_KEY"): | ||
| openai_key = await mock_settings_service.get_openai_key() | ||
| if openai_key: | ||
| os.environ["OPENAI_API_KEY"] = openai_key | ||
| except Exception: | ||
| error_logged = True # In real code, this logs a warning | ||
|
|
||
| # Should have caught the exception and continued | ||
| assert error_logged is True | ||
| # Env var should still be unset | ||
| assert os.environ.get("OPENAI_API_KEY") is None | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_partial_key_loading(self, monkeypatch) -> None: | ||
| """Verify partial key loading works (only one key in DB).""" | ||
| monkeypatch.delenv("OPENAI_API_KEY", raising=False) | ||
| monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) | ||
|
|
||
| # Mock settings service with only OpenAI key configured | ||
| mock_settings_service = AsyncMock() | ||
| mock_settings_service.get_openai_key = AsyncMock(return_value="sk-test-openai-key") | ||
| mock_settings_service.get_anthropic_key = AsyncMock(return_value=None) | ||
|
|
||
| with patch( | ||
| "sibyl.services.settings.get_settings_service", return_value=mock_settings_service | ||
| ): | ||
| if not os.environ.get("OPENAI_API_KEY"): | ||
| openai_key = await mock_settings_service.get_openai_key() | ||
| if openai_key: | ||
| os.environ["OPENAI_API_KEY"] = openai_key | ||
|
|
||
| if not os.environ.get("ANTHROPIC_API_KEY"): | ||
| anthropic_key = await mock_settings_service.get_anthropic_key() | ||
| if anthropic_key: | ||
| os.environ["ANTHROPIC_API_KEY"] = anthropic_key | ||
|
|
||
| # Only OpenAI key should be set | ||
| assert os.environ.get("OPENAI_API_KEY") == "sk-test-openai-key" | ||
| assert os.environ.get("ANTHROPIC_API_KEY") is None | ||
|
|
||
| # Cleanup | ||
| monkeypatch.delenv("OPENAI_API_KEY", raising=False) | ||
|
|
||
|
|
||
| class TestSettingsHotReload: | ||
| """Tests for hot-reloading API keys when updated via webapp.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_settings_updates_env_var(self, monkeypatch) -> None: | ||
| """Verify updating settings also updates os.environ.""" | ||
| monkeypatch.delenv("OPENAI_API_KEY", raising=False) | ||
|
|
||
| # Simulate the logic from settings.py update endpoint | ||
| new_key = "sk-new-openai-key" | ||
| os.environ["OPENAI_API_KEY"] = new_key | ||
|
|
||
| assert os.environ.get("OPENAI_API_KEY") == new_key | ||
|
|
||
| # Cleanup | ||
| monkeypatch.delenv("OPENAI_API_KEY", raising=False) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_settings_resets_graph_client(self) -> None: | ||
| """Verify GraphClient is reset after API key update.""" | ||
| reset_called = False | ||
|
|
||
| async def mock_reset_graph_client(): | ||
| nonlocal reset_called | ||
| reset_called = True | ||
|
|
||
| with patch( | ||
| "sibyl_core.graph.client.reset_graph_client", side_effect=mock_reset_graph_client | ||
| ): | ||
| # Simulate the reset call from settings.py | ||
| from sibyl_core.graph.client import reset_graph_client | ||
|
|
||
| await reset_graph_client() | ||
|
|
||
| assert reset_called is True | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_delete_setting_clears_env_var(self, monkeypatch) -> None: | ||
| """Verify deleting a setting clears it from os.environ.""" | ||
| monkeypatch.setenv("OPENAI_API_KEY", "sk-to-be-deleted") | ||
|
|
||
| # Simulate the logic from delete_setting endpoint | ||
| os.environ.pop("OPENAI_API_KEY", None) | ||
|
|
||
| assert os.environ.get("OPENAI_API_KEY") is None | ||
0pfleet marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: hyperb1iss/sibyl
Length of output: 1469
🏁 Script executed:
Repository: hyperb1iss/sibyl
Length of output: 1682
🏁 Script executed:
Repository: hyperb1iss/sibyl
Length of output: 1835
🏁 Script executed:
Repository: hyperb1iss/sibyl
Length of output: 3642
🏁 Script executed:
Repository: hyperb1iss/sibyl
Length of output: 2355
🏁 Script executed:
Repository: hyperb1iss/sibyl
Length of output: 1724
🏁 Script executed:
Repository: hyperb1iss/sibyl
Length of output: 1438
🏁 Script executed:
Repository: hyperb1iss/sibyl
Length of output: 1041
Wrap
load_api_keys_from_db()in a try/except to match the resilience of other startup steps.The cleanup helpers (
_cleanup_stale_working_agents,_cleanup_orphaned_agent_jobs) each have try/except blocks that allow the worker to start regardless of failures. Additionally, the main server startup guards this call withif db_connected:before invoking it. The worker startup currently has neither guard nor exception handling—if the DB is unreachable whenload_api_keys_from_db()runs (e.g.,get_settings_service()raises), the worker will crash. Either add a try/except block matching the cleanup helper pattern, or add theif db_connected:guard as used in the main startup.🤖 Prompt for AI Agents