fix: route strategy health to issue-only optimization monitoring - #107
Conversation
Co-Authored-By: Codex <noreply@openai.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab4e4bafbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| drift_results, | ||
| create_issues_for_domain, | ||
| monitoring_findings = _build_monitoring_findings(strategies, drift_results) | ||
| optimization_watch = dispatch_strategy_watch_findings( |
There was a problem hiding this comment.
When gh is missing or cannot be executed, list_open_issue_urls raises an OSError/FileNotFoundError, which the watcher helper does not convert into its errors result; this unguarded call therefore terminates health_cycle.py before optimization_error_lines and _send_telegram run. Catch watcher-level exceptions here and represent them as an issue-record failure so the promised fail-closed Telegram notification is still delivered.
AGENTS.md reference: ops/quant-monitor/AGENTS.md:L45-L46
Useful? React with 👍 / 👎.
| level = BriefingAction.TELEGRAM | ||
| kind = "runtime_risk" | ||
| severity = "high" |
There was a problem hiding this comment.
When one strategy has both degradation evidence and a circuit-breaker flag, these assignments replace its strategy_monitoring kind and GitHub level with a single runtime_risk Telegram finding. _strategy_monitoring_findings consequently filters it out, so the Telegram alert is sent but the status, score, and drift evidence is never recorded in the optimization issue queue; emit or preserve a separate monitoring finding while still escalating the runtime risk.
Useful? React with 👍 / 👎.
| finding | ||
| for finding in result.findings | ||
| if finding.level == BriefingAction.GITHUB_ISSUE | ||
| and not (finding.kind == "strategy_monitoring" and finding.strategy_profile) |
There was a problem hiding this comment.
When a report contains only a strategy summary, as supported by the summary-only consumer path, its strategy_monitoring_summary finding passes this filter and is sent through the generic issue creator rather than the strategy watcher. On the provided VPS configuration that generic creator uses QSL_GITHUB_REPO=QuantStrategyLab/CnEquityStrategies, so summaries for US, HK, or crypto are recorded in the wrong repository and also bypass watcher deduplication; route these findings using their domain instead.
Useful? React with 👍 / 👎.
| try: | ||
| score = float(row.get("overall_score")) | ||
| except (TypeError, ValueError): | ||
| continue |
There was a problem hiding this comment.
When a lifecycle row is review or critical but overall_score is missing or non-numeric, this early continue drops the row before its status is considered; the dashboard normalizer permits such rows by retaining the status and representing an invalid score as null. A status-only degradation therefore produces neither an optimization issue nor an operational error, so classify status independently of score parsing.
AGENTS.md reference: ops/quant-monitor/AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
| alert_identities.append( | ||
| f"optimization_error:issue_record_failed:{optimization_errors}" | ||
| ) |
There was a problem hiding this comment.
When consecutive cycles each have the same number of issue-record failures but for different repositories or strategies, this identity is unchanged, so _is_duplicate_alert suppresses the later Telegram alert as if it were the same incident. Include the failed repository and watcher issue key in the fingerprint so a new delivery failure cannot be hidden merely because its count matches the preceding cycle.
AGENTS.md reference: ops/quant-monitor/AGENTS.md:L45-L46
Useful? React with 👍 / 👎.
1 participant
Summary
Root cause
The VPS monitor still implemented an older routing rule where low health scores and critical drift were treated as human Telegram incidents. Its generic issue path also used repository labels that are not present on the target repositories, so the intended durable record could fail.
Safety
Validation
python3 -m ruff check .python3 -m pytest tests ops/quant-monitor/tests -q(646 passed, 1 skipped, 58 subtests passed)