Feat/streamlit llm key UI #4520
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🤖 Keepalive Loop StatusPR #4520 | Agent: Codex | Iteration 0/5 Current State
🔍 Failure Classification| Error type | infrastructure | |
|
Status | ✅ no new diagnostics |
|
Autofix updated these files:
|
There was a problem hiding this comment.
Pull request overview
This PR adds Streamlit UI support for managing LLM API keys and modernizes the validation approach for analysis configuration, along with extensive code formatting updates.
Changes:
- Adds a new
_default_api_keyfunction to resolve API keys from Streamlit secrets, environment variables, or provider-specific keys - Replaces config validation from
validate_configtovalidate_payloadusing a new bridge module - Removes hard entry threshold filtering logic from multi-period portfolio selection (appears unintentional)
- Adds
.streamlit/secrets.tomltemplate file for local API key configuration - Extensive code formatting improvements across multiple files
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.streamlit/secrets.toml |
New template file for storing API keys locally (not in .gitignore) |
streamlit_app/components/explain_results.py |
Adds _default_api_key function to resolve API keys from multiple sources; improves UI default value handling |
streamlit_app/components/analysis_runner.py |
Modernizes validation to use validate_payload bridge module; adds CSV path resolution for validation |
tests/app/test_analysis_runner_validation.py |
Updates test to work with new validation API |
src/trend_analysis/multi_period/replacer.py |
Formatting changes and removes hard entry barrier logic (lines 178-182 deleted) |
src/trend_analysis/multi_period/engine.py |
Formatting changes, removes hard entry filtering in multiple locations, adds weight policy fallback |
src/trend_analysis/export/__init__.py |
Formatting changes and defensive None checks for missing stats |
Comments suppressed due to low confidence (4)
streamlit_app/components/explain_results.py:167
- The function uses bare except clauses (except Exception) when accessing st.secrets. While this prevents the application from crashing if secrets are not configured, it silently swallows all exceptions including unexpected errors.
Consider catching more specific exceptions (e.g., KeyError, AttributeError) or at least logging the exception when it occurs. This would help with debugging if there are actual issues with the secrets configuration rather than just missing keys.
if secrets_key:
return secrets_key
env_key = os.environ.get("TREND_LLM_API_KEY")
if env_key:
return env_key
if provider_name == "openai":
try:
secrets_openai = st.secrets.get("OPENAI_API_KEY")
except Exception:
secrets_openai = None
if secrets_openai:
return secrets_openai
return os.environ.get("OPENAI_API_KEY")
if provider_name == "anthropic":
try:
secrets_anthropic = st.secrets.get("ANTHROPIC_API_KEY")
except Exception:
secrets_anthropic = None
if secrets_anthropic:
return secrets_anthropic
return os.environ.get("ANTHROPIC_API_KEY")
return None
def _build_result_chain(
provider: str | None = None,
*,
api_key: str | None = None,
src/trend_analysis/multi_period/replacer.py:180
- The hard entry barrier logic has been removed. Previously, funds with z-scores below z_entry_hard were immediately rejected from entry consideration. This removal changes the selection behavior significantly - funds can now be added even if they don't meet the hard threshold requirement.
This appears to be an unintentional change that was likely meant to be part of formatting-only updates. If this removal is intentional, it should be documented in the PR description and tested. If not, the logic should be restored to maintain the original entry filtering behavior.
else:
self._entry_strikes[f_str] = 0
# Classify candidates
src/trend_analysis/multi_period/engine.py:543
- The hard entry threshold filtering logic has been removed from the score frame selection. Previously, when a rebalancer had a hard z-entry threshold configured, funds with z-scores below this threshold were filtered out before selection. This removal changes the portfolio selection behavior.
This appears to be an unintentional change bundled with formatting updates. If intentional, it should be documented and tested. Otherwise, restore the filtering logic to maintain consistent entry barrier enforcement across the codebase.
)
target_weights = target_series.to_frame("weight")
src/trend_analysis/multi_period/engine.py:1864
- The _filter_entry_frame function previously filtered out funds below the z_entry_hard threshold before applying bottom_k filtering. This hard threshold check has been removed, changing the entry filtering behavior for new fund candidates.
This is part of a pattern of hard entry threshold removal across the codebase. If this is intentional, the change should be documented in the PR description with rationale. If not, the logic should be restored to maintain the hard entry barrier functionality.
in_cooldown = set(cooldowns or {})
candidates = [
str(c)
No description provided.