feat(#295): background PR lifecycle monitor with auto-retry#333
Open
xsovad06 wants to merge 5 commits into
Open
feat(#295): background PR lifecycle monitor with auto-retry#333xsovad06 wants to merge 5 commits into
xsovad06 wants to merge 5 commits into
Conversation
WalkthroughChangesThe PR adds a configurable background PR monitor that polls open PR states, sends notifications on configured transitions, retries CodeRabbit reviews after rate limits clear, and integrates task startup and shutdown into the dashboard lifecycle. PR monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
xsovad06
commented
Jul 14, 2026
xsovad06
left a comment
xsovad06
left a comment
Owner
Author
There was a problem hiding this comment.
Code Review for PR #333
The implementation is solid but has performance and error handling gaps. Most critical: rate limit checks are sequential (5/10) causing O(N) latency per cycle, and missing exception handling around resolve_gh_env() (5/10) could lose error context. Test coverage is comprehensive for core logic but missing integration tests for multi-project mode (4/10). The _NOTIFY_STATES string-based config lookup is fragile and could hide bugs (6/10). Documentation assumes GitHub API comment ordering without stating it (3/10).
8 findings (8 inline, 0 in summary)
| Sev | Category | File | Finding |
|---|---|---|---|
| 6/10 | bug | sova/supervisor/pr_monitor.py:36 |
The _NOTIFY_STATES dict maps state to config flag name as strings, then uses getattr(self.monitor_config, config_flag, False). If a future change adds a new state to the dict but misspells the config field name, the getattr silently returns False instead of raising AttributeError, hiding the bug. |
| 5/10 | performance | sova/supervisor/pr_monitor.py:87 |
Rate limit checks are executed sequentially for all PRs. With many open PRs (20+), this creates significant overhead. If there are 50 PRs and each gh pr view call takes 0.5s, the cycle takes 25s out of a 120s interval (20% overhead). |
| 5/10 | error-handling | sova/supervisor/pr_monitor.py:166 |
resolve_gh_env() call is not wrapped in try/except. If it raises (e.g., due to missing gh auth or subprocess errors), the exception propagates to the generic handler in run_loop(), losing PR-specific context in logs. |
| 4/10 | testing | tests/test_pr_monitor.py:1 |
No test verifies behavior when resolve_gh_env() raises an exception in _retry_coderabbit_review(). The code path exists (async I/O can fail), but there's no test ensuring it's handled gracefully. |
| 4/10 | testing | sova/dashboard/app.py:283 |
No integration test for multi-project mode path in the lifespan hook. The code spawns multiple PRMonitor tasks for different projects, but this is never tested. If _load_mon_cfg() or PRMonitor() construction fails for one project, it could crash the entire lifespan startup. |
| 3/10 | docs | sova/supervisor/pr_monitor.py:215 |
The code assumes GitHub API returns comments in chronological order (oldest-first), so reversed(comments) gives newest-first. This assumption is correct per GitHub API docs, but is not documented. If the API behavior changes or if different endpoints have different ordering, this logic breaks silently. |
| 3/10 | design | sova/supervisor/pr_monitor.py:53 |
The monitor creates a new PRSnapshot for every PR on every cycle, even when nothing changed. For projects with many stable PRs, this creates unnecessary allocations. The snapshots are lightweight, but at scale (100+ PRs) this adds up. |
| 2/10 | spec_alignment | sova/config/models.py:394 |
Config field is named poll_interval but spec's Implementation Notes refer to it as pr_poll_interval_seconds. While not a bug (env var is correctly SOVA_PR_MONITOR_POLL_INTERVAL), this creates confusion when reading spec vs code. |
Assessment: REVISE -- actionable findings should be addressed
…tion - Define _LABEL_POLL_INTERVAL constant in settings_meta.py instead of duplicating "Poll interval (s)" across ci, external_reviews, and pr_monitor setting entries (SonarCloud S1192). - Remove redundant json.JSONDecodeError from except clause in pr_monitor.py since it derives from ValueError which is already caught (SonarCloud S5765).
…_monitor - Test run_loop polls and re-raises CancelledError (lines 62-67) - Test run_loop catches non-cancellation exceptions and continues (lines 68-70) - Test _retry_coderabbit_review logs warning on gh failure (line 181) - Test _is_coderabbit_rate_limited returns False on malformed JSON (lines 217-218) Coverage: sova/supervisor/pr_monitor.py 89% -> 100%
Closes #295
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@sova/dashboard/app.py`:
- Around line 283-328: The multi-project PR-monitor wiring is duplicated in
production and tests, so the test does not exercise the real lifespan behavior.
In sova/dashboard/app.py lines 283-328, extract the config loading, exception
isolation, enable/repository gating, and PRMonitor construction from the
lifespan into a small testable helper, or expose the real lifespan path for
testing. In tests/test_pr_monitor.py lines 749-800, replace the hand-rolled loop
with that helper or lifespan invocation while preserving the assertions that one
monitor is created and its repo is "owner/good".
In `@sova/supervisor/pr_monitor.py`:
- Around line 96-110: Update the exception-handling branch in the rate-limit
results loop around _is_coderabbit_rate_limited to pass the caught result
exception instance to log.debug’s exc_info parameter instead of True, preserving
the existing rate_limits fallback and log context.
- Around line 147-202: The _retry_coderabbit_review method bypasses CodeRabbit
quota accounting when it posts a retry review. After a successful retry comment,
update the existing CodeRabbit quota cache or invoke its established
sync/recording mechanism so coderabbit_quota and pr_throttle immediately reflect
the retry; do not count failed gh commands.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 04e8f05f-700d-4c87-9f4b-3f89ed1ca45f
📒 Files selected for processing (6)
sova/config/loader.pysova/config/models.pysova/dashboard/app.pysova/dashboard/settings_meta.pysova/supervisor/pr_monitor.pytests/test_pr_monitor.py
Comment on lines
+283
to
+328
|
|
||
| # PR monitor background loop | ||
| pr_monitor_tasks: list[asyncio.Task] = [] | ||
| if is_multi: | ||
| from sova.config.loader import load_config as _load_mon_cfg | ||
|
|
||
| for path_str in list_projects().values(): | ||
| p = Path(path_str) | ||
| if not p.is_dir(): | ||
| continue | ||
| try: | ||
| pcfg = _load_mon_cfg(p) | ||
| except Exception: | ||
| log.warning("pr_monitor.config_load_failed", project=str(p), exc_info=True) | ||
| continue | ||
| if not pcfg.pr_monitor.enabled or not pcfg.github_repo: | ||
| continue | ||
|
|
||
| from sova.supervisor.pr_monitor import PRMonitor | ||
|
|
||
| monitor = PRMonitor( | ||
| project_dir=p, | ||
| monitor_config=pcfg.pr_monitor, | ||
| notification_config=pcfg.notification, | ||
| repo=pcfg.github_repo, | ||
| github_user=pcfg.github_user, | ||
| ) | ||
| pr_monitor_tasks.append(asyncio.create_task(monitor.run_loop())) | ||
| elif cfg.pr_monitor.enabled and cfg.github_repo: | ||
| from sova.supervisor.pr_monitor import PRMonitor | ||
|
|
||
| monitor = PRMonitor( | ||
| project_dir=resolved, | ||
| monitor_config=cfg.pr_monitor, | ||
| notification_config=cfg.notification, | ||
| repo=cfg.github_repo, | ||
| github_user=cfg.github_user, | ||
| ) | ||
| pr_monitor_tasks.append(asyncio.create_task(monitor.run_loop())) | ||
|
|
||
| yield | ||
| for t in pr_throttle_tasks: | ||
| bg_tasks = pr_throttle_tasks + pr_monitor_tasks | ||
| for t in bg_tasks: | ||
| t.cancel() | ||
| if pr_throttle_tasks: | ||
| await asyncio.gather(*pr_throttle_tasks, return_exceptions=True) | ||
| if bg_tasks: | ||
| await asyncio.gather(*bg_tasks, return_exceptions=True) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
No test exercises the real multi-project PR-monitor lifespan wiring. sova/dashboard/app.py's multi-project PR monitor loop (config load, exception isolation, enable/repo gating, task creation) is never invoked by a test through create_app/lifespan; the closest test reimplements this logic by hand instead of calling it, so the two can silently diverge.
sova/dashboard/app.py#L283-L328: extract the multi-project config-load + gating +PRMonitorconstruction loop into a small testable helper (or invokelifespandirectly in a test with mockedlist_projects/load_config) so production logic is what's actually asserted on.tests/test_pr_monitor.py#L749-L800: replace the hand-rolled reimplementation with a call into the extracted helper (or the reallifespan), keeping the same assertions (len(monitors) == 1,monitors[0].repo == "owner/good").
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 295-295: Do not catch blind exception: Exception
(BLE001)
📍 Affects 2 files
sova/dashboard/app.py#L283-L328(this comment)tests/test_pr_monitor.py#L749-L800
🤖 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 `@sova/dashboard/app.py` around lines 283 - 328, The multi-project PR-monitor
wiring is duplicated in production and tests, so the test does not exercise the
real lifespan behavior. In sova/dashboard/app.py lines 283-328, extract the
config loading, exception isolation, enable/repository gating, and PRMonitor
construction from the lifespan into a small testable helper, or expose the real
lifespan path for testing. In tests/test_pr_monitor.py lines 749-800, replace
the hand-rolled loop with that helper or lifespan invocation while preserving
the assertions that one monitor is created and its repo is "owner/good".
Comment on lines
+96
to
+110
| rate_limits: dict[int, bool] = {} | ||
| if self.monitor_config.auto_retry_coderabbit and prs: | ||
| tasks = { | ||
| pr["number"]: asyncio.ensure_future( | ||
| _is_coderabbit_rate_limited(pr["number"], repo=self.repo, github_user=self.github_user) | ||
| ) | ||
| for pr in prs | ||
| } | ||
| results = await asyncio.gather(*tasks.values(), return_exceptions=True) | ||
| for number, result in zip(tasks.keys(), results): | ||
| if isinstance(result, BaseException): | ||
| log.debug("pr_monitor.rate_limit_check_failed", pr=number, exc_info=True) | ||
| rate_limits[number] = False | ||
| else: | ||
| rate_limits[number] = result |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does structlog's format_exc_info processor accept an exception instance passed as exc_info, or only True/exc_info tuple?
💡 Result:
Yes, structlog's format_exc_info processor accepts an exception instance as the value for the exc_info key in the event dictionary [1][2][3]. When the processor encounters an exc_info key, it processes the value in one of three ways [1][3]: 1. If the value is a tuple, it uses it as an exception info tuple (equivalent to the result of sys.exc_info) [1][3]. 2. If the value is an exception instance (e.g., ValueError), it renders it directly into the exception field [1][3]. 3. If the value is true (but not a tuple or an exception instance), it obtains the exception information itself using sys.exc_info and renders that [1][3]. This behavior allows flexibility in how exception data is passed to the processor [3]. If no exc_info key is present or the value is false, the event dictionary is left untouched [1][3].
Citations:
- 1: https://github.com/hynek/structlog/blob/master/src/structlog/processors.py
- 2: https://structlog.org/en/21.3.0/api.html
- 3: https://www.structlog.org/en/25.4.0/exceptions.html
Pass the caught exception to exc_info. exc_info=True uses sys.exc_info() here, so the traceback from result is dropped; pass the exception instance instead.
Fix
if isinstance(result, BaseException):
- log.debug("pr_monitor.rate_limit_check_failed", pr=number, exc_info=True)
+ log.debug("pr_monitor.rate_limit_check_failed", pr=number, exc_info=result)
rate_limits[number] = False📝 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
| rate_limits: dict[int, bool] = {} | |
| if self.monitor_config.auto_retry_coderabbit and prs: | |
| tasks = { | |
| pr["number"]: asyncio.ensure_future( | |
| _is_coderabbit_rate_limited(pr["number"], repo=self.repo, github_user=self.github_user) | |
| ) | |
| for pr in prs | |
| } | |
| results = await asyncio.gather(*tasks.values(), return_exceptions=True) | |
| for number, result in zip(tasks.keys(), results): | |
| if isinstance(result, BaseException): | |
| log.debug("pr_monitor.rate_limit_check_failed", pr=number, exc_info=True) | |
| rate_limits[number] = False | |
| else: | |
| rate_limits[number] = result | |
| rate_limits: dict[int, bool] = {} | |
| if self.monitor_config.auto_retry_coderabbit and prs: | |
| tasks = { | |
| pr["number"]: asyncio.ensure_future( | |
| _is_coderabbit_rate_limited(pr["number"], repo=self.repo, github_user=self.github_user) | |
| ) | |
| for pr in prs | |
| } | |
| results = await asyncio.gather(*tasks.values(), return_exceptions=True) | |
| for number, result in zip(tasks.keys(), results): | |
| if isinstance(result, BaseException): | |
| log.debug("pr_monitor.rate_limit_check_failed", pr=number, exc_info=result) | |
| rate_limits[number] = False | |
| else: | |
| rate_limits[number] = result |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 105-105: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 107-107: exc_info= outside exception handlers
Remove exc_info=
(LOG014)
🤖 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 `@sova/supervisor/pr_monitor.py` around lines 96 - 110, Update the
exception-handling branch in the rate-limit results loop around
_is_coderabbit_rate_limited to pass the caught result exception instance to
log.debug’s exc_info parameter instead of True, preserving the existing
rate_limits fallback and log context.
Source: Linters/SAST tools
Comment on lines
+147
to
+202
| was_rate_limited = prev.rate_limited if prev else False | ||
| if was_rate_limited and not curr.rate_limited and self.monitor_config.auto_retry_coderabbit: | ||
| await self._retry_coderabbit_review(curr.number) | ||
|
|
||
| def _maybe_notify(self, snapshot: PRSnapshot) -> None: | ||
| """Send a notification if the new state is one we care about.""" | ||
| config_flag = _NOTIFY_STATES.get(snapshot.computed_state) | ||
| if not config_flag: | ||
| return | ||
| if not getattr(self.monitor_config, config_flag, False): | ||
| return | ||
|
|
||
| from sova.ipc.notifications import notify | ||
|
|
||
| state_label = _STATE_LABELS.get(snapshot.computed_state, snapshot.computed_state) | ||
| notify( | ||
| self.notification_config, | ||
| title="SOVA", | ||
| subtitle=f"PR #{snapshot.number} {state_label}", | ||
| message=snapshot.title, | ||
| group=f"sova-pr-{snapshot.number}", | ||
| ) | ||
| log.info( | ||
| "pr_monitor.notified", | ||
| pr=snapshot.number, | ||
| state=snapshot.computed_state, | ||
| ) | ||
|
|
||
| async def _retry_coderabbit_review(self, pr_number: int) -> None: | ||
| """Post @coderabbitai review comment to trigger a re-review.""" | ||
| from sova.utils.gh import resolve_gh_env | ||
| from sova.utils.shell import run | ||
|
|
||
| log.info("pr_monitor.retry_coderabbit", pr=pr_number) | ||
| try: | ||
| env = await resolve_gh_env(self.github_user) if self.github_user else None | ||
| except Exception: | ||
| log.warning("pr_monitor.gh_env_failed", pr=pr_number, exc_info=True) | ||
| return | ||
| result = await run( | ||
| "gh", | ||
| "pr", | ||
| "comment", | ||
| str(pr_number), | ||
| "--repo", | ||
| self.repo, | ||
| "--body", | ||
| "@coderabbitai review", | ||
| env=env, | ||
| ) | ||
| if not result.success: | ||
| log.warning( | ||
| "pr_monitor.retry_coderabbit_failed", | ||
| pr=pr_number, | ||
| stderr=result.stderr[:200], | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files 'sova/supervisor/pr_monitor.py' 'sova/dashboard/app.py' 'sova/**/pr_throttle*' 'sova/**/quota*' 'sova/**/coderabbit*'
printf '\n## Outline: pr_monitor.py\n'
ast-grep outline sova/supervisor/pr_monitor.py --view expanded || true
printf '\n## Outline: dashboard/app.py\n'
ast-grep outline sova/dashboard/app.py --view expanded || true
printf '\n## Search for quota/throttle wiring\n'
rg -n --hidden --glob 'sova/**/*.py' 'coderabbit_quota|pr_throttle|quota|throttle|review trigger|coderabbitai review|`@coderabbitai` review' sova
printf '\n## Relevant slices\n'
sed -n '1,260p' sova/supervisor/pr_monitor.py
printf '\n--- dashboard/app.py ---\n'
sed -n '200,320p' sova/dashboard/app.pyRepository: xsovad06/sova
Length of output: 24362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## pr_throttle.py (relevant slices)\n'
sed -n '1,220p' sova/supervisor/pr_throttle.py
printf '\n## coderabbit_quota.py (relevant slices)\n'
sed -n '1,220p' sova/supervisor/coderabbit_quota.py
printf '\n---\n'
sed -n '220,420p' sova/supervisor/coderabbit_quota.py
printf '\n## create_pr.py (quota usage)\n'
sed -n '100,210p' sova/core/steps/create_pr.pyRepository: xsovad06/sova
Length of output: 22739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## record_event / sync_from_github usage\n'
rg -n --hidden --glob 'sova/**/*.py' 'record_event\(|sync_from_github\(' sova
printf '\n## quota-related routes and docs\n'
sed -n '1,180p' sova/dashboard/routers/quota.py
printf '\n---\n'
sed -n '1,220p' sova/supervisor/coderabbit_quota.py
printf '\n## pr_monitor references to quota/review tracking\n'
rg -n --hidden --glob 'sova/**/*.py' 'auto_retry_coderabbit|coderabbit_quota|review comment|`@coderabbitai` review|rate_limited' sova/supervisor/pr_monitor.py sova/dashboard/app.py sova/core sova/dashboardRepository: xsovad06/sova
Length of output: 14394
Retry path skips quota accounting
_retry_coderabbit_review() posts @coderabbitai review directly, but it never updates the CodeRabbit quota cache. With coderabbit_quota enabled, the local window will undercount retry-driven reviews until an explicit sync runs, which can let pr_throttle admit more PRs than the real quota allows. Consider syncing or recording the retry result here.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 183-183: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@sova/supervisor/pr_monitor.py` around lines 147 - 202, The
_retry_coderabbit_review method bypasses CodeRabbit quota accounting when it
posts a retry review. After a successful retry comment, update the existing
CodeRabbit quota cache or invoke its established sync/recording mechanism so
coderabbit_quota and pr_throttle immediately reflect the retry; do not count
failed gh commands.
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
1 participant
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.
Summary
Changes
Core monitor (
sova/supervisor/pr_monitor.py):PRMonitorclass with async polling loop following WatchLoop pattern@coderabbitai reviewwhen rate-limited PRs have quota availableConfiguration (
sova/config/models.py,loader.py,settings_meta.py):SupervisorConfigsection withpr_monitor_enabled,pr_poll_interval_seconds, notification toggles, andauto_retry_rate_limitedIntegration (
sova/dashboard/app.py):config.supervisor.pr_monitor_enabledTests (
tests/test_pr_monitor.py):Review guidance
Focus on:
_detect_transition()-- does it correctly identify when to notify vs. stay silent?coderabbit_quota,pr_service) -- any missing edge cases?Trade-offs:
Test plan
make dev, create/update PRs, observe monitor logs and notificationssova server stopCloses #295