feat(core): CI failure auto-recovery in MonitorCIStep#95
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds CI run-id/log helpers, CI polling, and a bounded LLM-driven fix-and-retry loop inside MonitorCIStep (configurable via CIConfig.max_fix_attempts, capped at 3); exposes pre-push-hook discovery; adds tests and docs; persists file-based handoffs into TaskRun finalization and returns a default "revise" verdict when handoff_json is missing. ChangesCI Failure Auto-Recovery
Agent Dashboard Improvements
4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sova/core/steps/monitor_ci.py (1)
121-235: 💤 Low valueConsider extracting sub-methods to reduce cognitive complexity.
SonarCloud flags cognitive complexity of 22 (vs allowed 15). While the logic is correct and follows a clear sequential flow, consider extracting helper methods for the main phases (e.g.,
_invoke_llm_fix,_validate_and_push) in a future cleanup pass. This is not blocking since the current structure is readable and each phase has distinct error handling needs.🤖 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/core/steps/monitor_ci.py` around lines 121 - 235, The method _try_fix_ci has high cognitive complexity—extract the distinct phases into small helpers to simplify it: create _invoke_llm_fix(ctx, failed_checks, recent_commits) to call get_ci_failure_logs and invoke (returning the llm_result and cost), create _validate_pre_push_and_run(hook_path, ctx) to locate _find_pre_push_hook and run the hook and surface hook output, and create _push_and_poll(ctx, branch_name) to call git_ops.push and then self._poll_ci; replace the inlined blocks in _try_fix_ci with calls to these helpers and preserve existing error handling and return semantics (keep using ctx.add_cost, total_fix_cost, and how failures are returned).Source: Linters/SAST tools
🤖 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/git/pr.py`:
- Around line 353-383: The truncation logic can slice with a non-positive
remaining (remaining == 0) causing output[-remaining:] to return the whole
string; before performing the slice in the loop over failed_checks, explicitly
handle non-positive remaining (the variable initialized from max_log_chars) by
skipping further processing or breaking out of the loop when remaining <= 0, or
by clamping remaining to a positive value so the subsequent output =
output[-remaining:] cannot return the full log; update the block that uses
remaining, output, and the slice (look for remaining = max_log_chars, the loop
over failed_checks, and the output[-remaining:] expression) to enforce this
guard.
---
Nitpick comments:
In `@sova/core/steps/monitor_ci.py`:
- Around line 121-235: The method _try_fix_ci has high cognitive
complexity—extract the distinct phases into small helpers to simplify it: create
_invoke_llm_fix(ctx, failed_checks, recent_commits) to call get_ci_failure_logs
and invoke (returning the llm_result and cost), create
_validate_pre_push_and_run(hook_path, ctx) to locate _find_pre_push_hook and run
the hook and surface hook output, and create _push_and_poll(ctx, branch_name) to
call git_ops.push and then self._poll_ci; replace the inlined blocks in
_try_fix_ci with calls to these helpers and preserve existing error handling and
return semantics (keep using ctx.add_cost, total_fix_cost, and how failures are
returned).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8d95948-807a-4bef-ae0c-51b848804f85
📒 Files selected for processing (8)
.claude/rules/architecture.md.gitignorepyproject.tomlsova/config/models.pysova/core/steps/monitor_ci.pysova/git/operations.pysova/git/pr.pytests/test_core.py
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
c71a47e to
3207c92
Compare
|
Addressing CodeRabbit findings:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sova/core/steps/monitor_ci.py (1)
121-235: 💤 Low valueConsider extracting helper methods to reduce cognitive complexity.
SonarCloud flags cognitive complexity of 22 (allowed 15). While the logic is correct, the function handles budget checks, log fetching, LLM invocation, hook validation, pushing, and re-polling in a single method.
Potential extractions:
_invoke_llm_fix()for lines 165-180_validate_and_push()for lines 182-213This is optional given the feature is self-contained and well-tested.
🤖 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/core/steps/monitor_ci.py` around lines 121 - 235, The _try_fix_ci method is too complex (cognitive complexity 22); extract the LLM invocation and the pre-push validation+push logic into two helpers to simplify it: create an async _invoke_llm_fix(ctx, prompt, max_budget) that performs the invoke(...) call, handles RuntimeError, updates ctx.add_cost and returns (llm_result, error_or_none) or raises a controlled error; and create an async _validate_and_push(ctx, hook_path, branch_name, working_dir, max_attempts, attempt) that runs the pre-push hook if present, logs/warns on failures, performs git_ops.push with force/set_upstream, and returns (push_success_bool, hook_output_or_error) so _try_fix_ci can call these helpers in place of the inlined blocks (replace the invoke/ctx.add_cost block and the hook+git_ops.push block) to reduce branching and make _try_fix_ci flow linear.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@sova/core/steps/monitor_ci.py`:
- Around line 121-235: The _try_fix_ci method is too complex (cognitive
complexity 22); extract the LLM invocation and the pre-push validation+push
logic into two helpers to simplify it: create an async _invoke_llm_fix(ctx,
prompt, max_budget) that performs the invoke(...) call, handles RuntimeError,
updates ctx.add_cost and returns (llm_result, error_or_none) or raises a
controlled error; and create an async _validate_and_push(ctx, hook_path,
branch_name, working_dir, max_attempts, attempt) that runs the pre-push hook if
present, logs/warns on failures, performs git_ops.push with force/set_upstream,
and returns (push_success_bool, hook_output_or_error) so _try_fix_ci can call
these helpers in place of the inlined blocks (replace the invoke/ctx.add_cost
block and the hook+git_ops.push block) to reduce branching and make _try_fix_ci
flow linear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38acce6e-a787-4c29-87df-e986b1f69759
📒 Files selected for processing (8)
.claude/rules/architecture.md.gitignorepyproject.tomlsova/config/models.pysova/core/steps/monitor_ci.pysova/git/operations.pysova/git/pr.pytests/test_core.py
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- sova/git/operations.py
- .claude/rules/architecture.md
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- sova/config/models.py
- sova/git/pr.py
xsovad06
left a comment
xsovad06
left a comment
There was a problem hiding this comment.
PR Review: feat(core): CI failure auto-recovery in MonitorCIStep
Summary
PR #95 by @xsovad06 adds CI failure auto-recovery to MonitorCIStep. When CI checks fail, the step now enters a bounded fix loop: fetch CI failure logs, invoke Claude to diagnose and fix, validate locally via pre-push hooks, re-push with --force-with-lease, and re-poll CI. The loop is configurable via ci.max_fix_attempts (default 2, hard-capped at 3, set 0 to disable). 3 commits, 8 files changed, +557/-14 lines. All GitHub Actions CI checks pass. No linked GitHub Issue.
Ghost Commits
The author claimed the CodeRabbit remaining <= 0 guard fix was "squashed into the original commit." Verified -- the guard is present in the current code at sova/git/pr.py:381. No ghost commits found.
Findings
[MEDIUM] Consistency -- Cross-module private import of _find_pre_push_hook
Location: sova/core/steps/monitor_ci.py:120
Problem: _try_fix_ci imports _find_pre_push_hook from sova.core.steps.validate. The underscore prefix signals "module-internal" -- this function now has two callers across two modules but its name says "don't depend on me." A rename or refactor in validate.py would break monitor_ci.py without any warning since private functions have no stability contract.
Suggestion: Drop the underscore prefix -- rename to find_pre_push_hook() in validate.py. Both callers already exist, and the function's signature is stable.
Value: 4/10
[LOW] Consistency -- Inconsistent import paths for git.pr symbols
Location: sova/core/steps/monitor_ci.py:14
Problem: get_ci_checks is imported via the facade (sova.git.operations), but CICheck and get_ci_failure_logs are imported directly from the source module (sova.git.pr). The facade already re-exports get_ci_failure_logs and CICheck.
Suggestion: Import all three from the facade for consistency, or import all from source. Either way, be consistent.
Value: 2/10
[LOW] Robustness -- Section headers not counted in log budget
Location: sova/git/pr.py:385-386
Problem: remaining -= len(output) tracks only the log content. The prepended header (=== {check.name} (run {run_id}) ===\n) and the \n\n join separators are not counted. Total output can exceed max_log_chars by the sum of all headers.
Suggestion: Account for header length in the budget: remaining -= len(section) instead of remaining -= len(output). Or accept the minor overshoot as intentional -- the budget is soft and 8000 chars is already conservative.
Value: 1/10
Verdict
Approve -- This is solid work. The only finding above the fix threshold (4/10) is the private import naming, which is worth addressing but not blocking. The core logic is correct, well-tested, and follows the established ValidateStep pattern. The backward compatibility story is clean (max_fix_attempts=0 preserves old behavior), and the hard cap at 3 prevents runaway loops.
What's Done Well
-
Clean separation of concerns: extracting
_poll_cito be reusable between the initial check and post-fix re-checks is a good structural call. Theexecutemethod is now a clean dispatcher. -
Defensive budget tracking: checking
ctx.is_budget_exceededat the top of each attempt AND passingmax_budget_usdto the LLM call ensures both coarse and fine-grained cost control. Thectx.add_cost()call maintains authoritative budget state. -
Comprehensive test coverage: 8 test cases cover the happy path (1st and 2nd attempt success), exhaustion, disable path, budget guard, hook failure, push failure, and empty-log graceful degradation. The existing test was updated to set
max_fix_attempts=0so it exercises the pre-existing behavior explicitly. The mock targets are correctly placed (module-level for top-level imports, source-module for lazy imports).
-- Koda
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
3207c92 to
172cc69
Compare
|
All review findings addressed in the force-pushed commits: [MEDIUM] Cross-module private import (4/10): Fixed. Renamed [LOW] Inconsistent import paths (2/10): Fixed. [LOW] Section headers not counted in log budget (1/10): Fixed. Scout fix: Removed stale All fixes squashed into the original feature commits (no fix-on-fix). CI passed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sova/config/models.py (1)
65-65: ⚡ Quick winMake the runtime cap explicit in config validation.
MonitorCIStep.execute()already clamps this withmin(..., _MAX_CI_FIX_ATTEMPTS), so values above3are accepted here but silently ignored at runtime. Adding the upper bound here avoids misleadingsova.tomlsettings.Suggested change
- max_fix_attempts: int = Field(2, ge=0) + max_fix_attempts: int = Field(2, ge=0, le=3)🤖 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/config/models.py` at line 65, The config accepts max_fix_attempts without an upper bound while MonitorCIStep.execute clamps to _MAX_CI_FIX_ATTEMPTS; update the Field definition for max_fix_attempts in the models.py dataclass to include an upper bound (le=_MAX_CI_FIX_ATTEMPTS) so validation enforces the same runtime cap, referencing the same constant (_MAX_CI_FIX_ATTEMPTS) used by MonitorCIStep.execute to keep behavior consistent.
🤖 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/core/steps/monitor_ci.py`:
- Around line 214-227: The loop in monitor_ci calling _poll_ci currently reuses
stale failed_checks when _poll_ci returns new_failed == [], causing unnecessary
retries; after awaiting self._poll_ci(ctx) inside the monitor_ci flow, if
result.success return the StepResult as before, but if not result.success and
new_failed is empty then return the poll result immediately (do not update
failed_checks or continue retrying); only update failed_checks = new_failed and
log/continue the loop when new_failed contains concrete failures. Ensure you
reference the _poll_ci call, the result/new_failed tuple, and the failed_checks
variable when making this change.
- Around line 181-205: After running the pre-push hook (find_pre_push_hook and
run) but before calling git_ops.push, add a guard that ensures there is a new
commit to push and the working tree is clean: check that the working tree has no
unstaged/unstaged changes and that the local branch (ctx.branch_name) is ahead
of origin (e.g., compare HEAD vs origin/ctx.branch_name or use an existing
git_ops helper if available). If the working tree is dirty or the branch is not
ahead (no-op push), log a warning similar to "step.monitor_ci.no_new_commit"
with attempt and short status, and fail fast by returning a StepResult
indicating CI fix made no commit (use the same error/summary pattern as other
failures) instead of calling git_ops.push. Ensure this check is placed
immediately before git_ops.push so it prevents no-op pushes.
---
Nitpick comments:
In `@sova/config/models.py`:
- Line 65: The config accepts max_fix_attempts without an upper bound while
MonitorCIStep.execute clamps to _MAX_CI_FIX_ATTEMPTS; update the Field
definition for max_fix_attempts in the models.py dataclass to include an upper
bound (le=_MAX_CI_FIX_ATTEMPTS) so validation enforces the same runtime cap,
referencing the same constant (_MAX_CI_FIX_ATTEMPTS) used by
MonitorCIStep.execute to keep behavior consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be4f7a77-0814-4821-9909-045ebb3985a0
📒 Files selected for processing (9)
.claude/rules/architecture.md.gitignorepyproject.tomlsova/config/models.pysova/core/steps/monitor_ci.pysova/core/steps/validate.pysova/git/operations.pysova/git/pr.pytests/test_core.py
✅ Files skipped from review due to trivial changes (4)
- sova/core/steps/validate.py
- sova/git/operations.py
- .gitignore
- .claude/rules/architecture.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- sova/git/pr.py
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
172cc69 to
70fccf5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sova/core/steps/monitor_ci.py (1)
199-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire a clean worktree before force-pushing auto-fixes.
Line 206 only proves
HEADis ahead oforigin/<branch>. If the hook or the LLM leaves files modified after the last commit, this branch still pushes the old commit while the validated changes stay local, so the next CI poll can run against stale code.Suggested guard
+ status = await run("git", "status", "--porcelain", cwd=ctx.working_dir) ahead = await run( "git", "rev-list", "--count", f"origin/{ctx.branch_name}..HEAD", cwd=ctx.working_dir, ) - if not ahead.success or int(ahead.stdout.strip() or "0") == 0: - log.warning("step.monitor_ci.no_new_commit", attempt=attempt) + if ( + not status.success + or status.stdout.strip() + or not ahead.success + or int(ahead.stdout.strip() or "0") == 0 + ): + log.warning( + "step.monitor_ci.no_new_commit", + attempt=attempt, + dirty=bool(status.success and status.stdout.strip()), + ) if attempt == max_attempts: return StepResult( success=False, summary=f"CI fix produced no pushable commit after {max_attempts} attempt(s)", error="Auto-recovery needs a committed fix before pushing",🤖 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/core/steps/monitor_ci.py` around lines 199 - 214, The logic only checks that HEAD is ahead of origin/{ctx.branch_name} but doesn't ensure the worktree is clean, allowing uncommitted/modified files to be left out of a force-push; after computing `ahead` (in the same block using `run(...)` and before attempting any push or marking success) run a git cleanliness check (e.g., `git status --porcelain` or `git diff-index --quiet HEAD --`) against `ctx.working_dir` and, if the worktree is dirty, log a warning and treat it like "no new commit" (increment attempt or return a failing StepResult as done with `StepResult` in this function), so only when the repo is clean and HEAD is ahead do you proceed to push.
🤖 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.
Duplicate comments:
In `@sova/core/steps/monitor_ci.py`:
- Around line 199-214: The logic only checks that HEAD is ahead of
origin/{ctx.branch_name} but doesn't ensure the worktree is clean, allowing
uncommitted/modified files to be left out of a force-push; after computing
`ahead` (in the same block using `run(...)` and before attempting any push or
marking success) run a git cleanliness check (e.g., `git status --porcelain` or
`git diff-index --quiet HEAD --`) against `ctx.working_dir` and, if the worktree
is dirty, log a warning and treat it like "no new commit" (increment attempt or
return a failing StepResult as done with `StepResult` in this function), so only
when the repo is clean and HEAD is ahead do you proceed to push.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 097377a3-9e06-413b-9c3a-e66dea31780b
📒 Files selected for processing (6)
.claude/rules/architecture.md.gitignorepyproject.tomlsova/core/steps/monitor_ci.pysova/core/steps/validate.pytests/test_core.py
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- .claude/rules/architecture.md
🚧 Files skipped from review as they are similar to previous changes (1)
- sova/core/steps/validate.py
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
06e2550 to
e0ddf90
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sova/dashboard/services/agent_db.py (1)
122-123: 💤 Low valueConsider logging handoff persistence failures at warning level.
File handoff persistence failures are currently logged at debug level. Since this operation affects downstream recovery verdict logic (see
get_sova_review_verdict), consider logging at warning level for better visibility when troubleshooting verdict issues.📊 Proposed logging level change
except Exception: - log.debug("task_run.file_handoff_failed", exc_info=True) + log.warning("task_run.file_handoff_failed", exc_info=True)🤖 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/services/agent_db.py` around lines 122 - 123, The catch-all except block that currently calls log.debug("task_run.file_handoff_failed", exc_info=True) should be changed to log.warning so handoff persistence failures are visible; update the call to log.warning("task_run.file_handoff_failed", exc_info=True) (or log.warning with a short contextual message) in the same except block that surrounds the file handoff logic, referencing the "task_run.file_handoff_failed" log tag and ensuring exc_info=True remains so stack traces are preserved for downstream debugging (see interactions with get_sova_review_verdict).
🤖 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/services/agent_db.py`:
- Line 96: Update the _persist_file_handoff function signature to use the
TaskRun type instead of object by adding a forward-reference type hint (def
_persist_file_handoff(task_run: "TaskRun", project_dir: Path) -> None:) or,
alternatively, add a TYPE_CHECKING import and conditional import from
sova.db.models (if TYPE_CHECKING: from sova.db.models import TaskRun) and
annotate task_run as TaskRun; ensure you import TYPE_CHECKING from typing if
using that approach and remove the generic object annotation so the function
signature satisfies the project's type-hinting guideline.
- Around line 168-169: Blocking synchronous file read occurs because
_finalize_task_run calls _persist_file_handoff inside async with
session.begin(), and _persist_file_handoff uses
sova.ipc.handoff.read_handoff_file (Path.read_text) which blocks the event loop;
fix by moving the read outside the transaction or offloading it to a thread
(e.g., await asyncio.to_thread(read_handoff_file, ...)) before entering async
with session.begin(), then inside the transaction set task_run.handoff_json and
persist changes (keep _persist_file_handoff updated to accept pre-read handoff
data or replace its read with the threaded/read-outside logic and only perform
DB assignment within _finalize_task_run).
---
Nitpick comments:
In `@sova/dashboard/services/agent_db.py`:
- Around line 122-123: The catch-all except block that currently calls
log.debug("task_run.file_handoff_failed", exc_info=True) should be changed to
log.warning so handoff persistence failures are visible; update the call to
log.warning("task_run.file_handoff_failed", exc_info=True) (or log.warning with
a short contextual message) in the same except block that surrounds the file
handoff logic, referencing the "task_run.file_handoff_failed" log tag and
ensuring exc_info=True remains so stack traces are preserved for downstream
debugging (see interactions with get_sova_review_verdict).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a67afc7b-9a11-479e-9665-5a0b005aa0ec
📒 Files selected for processing (2)
sova/dashboard/services/agent_db.pysova/dashboard/services/agent_recovery.py
xsovad06
left a comment
xsovad06
left a comment
There was a problem hiding this comment.
Review: CI Auto-Recovery in MonitorCIStep
Overall this is a well-structured change that follows the established internal-loop pattern from ValidateStep. The fix loop, budget guards, and graceful degradation paths are sound. The handoff persistence fix for command:review-pr addresses a real gap. I have a few findings.
Findings
1. Test mock target mismatch for find_pre_push_hook -- tests may not be testing what they think (5/10)
_try_fix_ci does a lazy import: from sova.core.steps.validate import find_pre_push_hook. The tests patch sova.core.steps.validate.find_pre_push_hook -- which is correct for a lazy import from the source module. However, per the architecture rule on "Facade re-exports and patch.object", this only works because the import resolves from the source each time. This is fine and correct as written, but it is worth a comment in the test file clarifying why the patch target is on validate rather than on monitor_ci, to prevent future contributors from "fixing" it to sova.core.steps.monitor_ci.find_pre_push_hook (which would fail since it is not a module-level import).
No code change required -- informational only.
2. _persist_file_handoff uses object type annotation instead of TaskRun (3/10)
File: sova/dashboard/services/agent_db.py, line 96
def _persist_file_handoff(task_run: object, project_dir: Path) -> None:This is typed as object but the function body accesses .issue_number, .pr_number, .handoff_json, .id, and .source -- all TaskRun attributes. The object type defeats static analysis and IDE support. This is likely a workaround to avoid importing TaskRun at module scope (since it is imported inside _finalize_task_run), but since _persist_file_handoff is always called from inside that same try block where TaskRun is already imported, the annotation could use a string literal "TaskRun" or TYPE_CHECKING guard.
3. _persist_file_handoff is synchronous but read_handoff_file does file I/O (2/10)
_persist_file_handoff is a sync function called inside an async DB transaction. read_handoff_file reads from disk synchronously via path.read_text(). For a small JSON file this is negligible, and the function is wrapped in try/except so it cannot crash the caller. Acceptable as-is given the context (finalization path, single small file), but noting for awareness.
4. No test for _persist_file_handoff or the null-handoff fallback in get_sova_review_verdict (4/10)
The existing test_dashboard.py tests for get_sova_review_verdict all provide handoff_json data. There is no test for the new path where handoff_json is None but a status="done" review run exists (the fallback to verdict="revise"). Similarly, _persist_file_handoff has no test coverage. Both are important behavioral changes that affect the "Address Review" button visibility.
Recommend adding at minimum:
- A test for
get_sova_review_verdictwith aTaskRun(role="command:review-pr", status="done", handoff_json=None)to verify the "revise" fallback. - A unit test for
_persist_file_handoffwith matching/non-matching issue numbers.
5. _shell_side_effect does not validate the command shape (2/10)
File: tests/test_core.py, line 1539-1543
def _shell_side_effect(*args, **kwargs):
if "rev-list" in args:
return MagicMock(success=True, stdout="1\n")
return MagicMock(success=True, stdout="abc1234 fix\n")This dispatcher matches "rev-list" in args against the positional args, but run() is called with individual string args (run("git", "rev-list", "--count", ...)) so args is ("git", "rev-list", "--count", ...). The in check works on tuples, so "rev-list" in args checks for an exact element match -- this is correct. Just noting the pattern works because each arg is a separate string element.
6. _build_fix_prompt could leak large CI logs into the LLM prompt (2/10)
ci_logs is already bounded by max_log_chars=8000 in get_ci_failure_logs, so the prompt size is inherently capped. The recent_commits output from git log -5 is also bounded. No issue here -- this is well-handled.
7. Defensive coding: rev-list stdout parsing (3/10)
File: sova/core/steps/monitor_ci.py, line 206
if not ahead.success or int(ahead.stdout.strip() or "0") == 0:If ahead.stdout contains unexpected output (e.g., a git error message that is not empty but not a number), this will raise ValueError and propagate up uncaught from _try_fix_ci, which would crash the step. The run() call should report success=False for non-zero exit codes, but defensive wrapping would be safer:
try:
count = int(ahead.stdout.strip() or "0")
except ValueError:
count = 0This matches the defensive style used elsewhere in the codebase (e.g., the json.loads wrapping in get_ci_checks).
8. Architecture doc accurately reflects the implementation (0/10 -- no issue)
The new entry in architecture.md correctly describes the fix loop, cap, disable mechanism, and budget checking. The "File:" reference is accurate. Doc count ("16 BaseStep implementations") still holds since this is a modification to an existing step, not a new one. No action needed.
Summary
The CI auto-recovery feature is well-designed: it follows the proven ValidateStep internal-loop pattern, respects budget limits, validates locally before pushing, and fails gracefully at every point. The force=True correctly maps to --force-with-lease in push(). The handoff persistence fix addresses a real gap in the command:review-pr flow.
The main actionable items are:
- Finding 4: Add test coverage for the null-handoff fallback path and
_persist_file_handoff - Finding 7: Wrap the
int()parse in a try/except for robustness
Everything else is minor or informational. Solid work overall.
-- Koda
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
2 times, most recently
from
20ab1b2 to
da6b3ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
sova/dashboard/services/agent_db.py (1)
96-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid synchronous handoff file I/O in the async DB transaction.
read_handoff_file()is synchronous and is called duringasync with session.begin(), which can block the event loop in the dashboard path. Offload the read withasyncio.to_thread(...)(or pre-read before entering the transaction) and only persist the matchedhandoff_jsoninside the transaction.Proposed fix
+import asyncio @@ -def _persist_file_handoff(task_run: "TaskRun", project_dir: Path) -> None: # noqa: F821 +async def _persist_file_handoff(task_run: "TaskRun", project_dir: Path) -> None: # noqa: F821 @@ - handoff = read_handoff_file(project_dir) + handoff = await asyncio.to_thread(read_handoff_file, project_dir) @@ - _persist_file_handoff(task_run, agent.project_dir) + await _persist_file_handoff(task_run, agent.project_dir)As per coding guidelines, "sova/**/*.py: ... Check for: async context manager leaks ...".
Also applies to: 168-169
🤖 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/services/agent_db.py` around lines 96 - 107, The synchronous read_handoff_file call inside the async DB transaction blocks the event loop; change _persist_file_handoff to call read_handoff_file via asyncio.to_thread(...) (or read the handoff before entering async with session.begin()) so only the resulting handoff data (handoff_json) is written inside the transaction. Locate the read_handoff_file call in _persist_file_handoff (and the similar call later around the second usage) and move/await the to_thread call outside the transaction block so the session.begin() only performs the DB persist of the matched handoff_json.Source: Coding guidelines
🤖 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/core/steps/monitor_ci.py`:
- Around line 173-179: The except RuntimeError blocks in monitor_ci.py currently
call log.error("step.monitor_ci.llm_failed", error=str(exc)) (and the similar
block around lines 227-233) but omit traceback info; update both exception
handlers that catch RuntimeError in the monitor CI flow to call
log.exception("step.monitor_ci.llm_failed", extra={"error": str(exc)}) or pass
exc_info=True to the logger so the stack trace is preserved, keeping the
existing summary and StepResult return behavior (refer to the except
RuntimeError as exc handlers and the StepResult construction to locate the exact
spots).
- Around line 45-47: The current validate_output method unconditionally returns
GateCheckResult(passed=True); replace it with real checks that inspect the
unstaged diff, staged diff, and commits ahead of base via the ExecutionContext
(use the context methods that expose diffs/commits), aggregate any violations,
and return GateCheckResult(passed=bool_ok, details=errors) instead of always
true; specifically update validate_output(self, ctx: ExecutionContext) ->
GateCheckResult to (1) obtain unstaged and staged diffs from ctx, (2) obtain
commits ahead of base from ctx, (3) apply the step’s validation rules to those
three sources, and (4) return a GateCheckResult with passed=False and
appropriate messages when any rule fails and passed=True only when all checks
pass.
In `@sova/git/pr.py`:
- Around line 381-391: The returned joined sections currently ignores the "\n\n"
separators length and can exceed max_log_chars; adjust the budgeting logic in
the block that builds sections (variables: remaining, header, output, section,
sections) to account for the separator size when appending non-first
sections—for example, subtract len("\n\n") (or add that to the cost) from
remaining or from budget when sections is non-empty before deciding/truncating
output, and then decrement remaining by len(section) plus the separator length
when you append; ensure the first section does not pay the separator cost so the
final "\n\n".join(sections) cannot exceed the intended limit.
In `@tests/test_core.py`:
- Around line 1539-1543: The helper _shell_side_effect needs explicit type
annotations: change its signature to accept *args: Any and **kwargs: Any and
return a MagicMock (-> MagicMock); ensure typing.Any is imported and that
MagicMock is available from unittest.mock (add imports if missing) so the
function signature becomes _shell_side_effect(*args: Any, **kwargs: Any) ->
MagicMock and the body remains unchanged.
---
Duplicate comments:
In `@sova/dashboard/services/agent_db.py`:
- Around line 96-107: The synchronous read_handoff_file call inside the async DB
transaction blocks the event loop; change _persist_file_handoff to call
read_handoff_file via asyncio.to_thread(...) (or read the handoff before
entering async with session.begin()) so only the resulting handoff data
(handoff_json) is written inside the transaction. Locate the read_handoff_file
call in _persist_file_handoff (and the similar call later around the second
usage) and move/await the to_thread call outside the transaction block so the
session.begin() only performs the DB persist of the matched handoff_json.
🪄 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: bf07d17a-531c-49e5-b54e-79b9be2d0ccb
⛔ Files ignored due to path filters (1)
.claude/rules/architecture.mdis excluded by!.claude/**and included by none
📒 Files selected for processing (9)
sova/config/models.pysova/core/steps/monitor_ci.pysova/core/steps/validate.pysova/dashboard/services/agent_db.pysova/dashboard/services/agent_recovery.pysova/git/operations.pysova/git/pr.pytests/test_core.pytests/test_dashboard.py
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
da6b3ae to
f764761
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/test_core.py (1)
1539-1543:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd type annotations to
_shell_side_effecthelper.This helper is missing parameter and return type annotations. As per coding guidelines, type hints are required on all Python function signatures.
Proposed fix
+from typing import Any + -def _shell_side_effect(*args, **kwargs): +def _shell_side_effect(*args: Any, **_kwargs: Any) -> MagicMock: """Dispatch shell.run mock based on the command being called.""" if "rev-list" in args: return MagicMock(success=True, stdout="1\n") return MagicMock(success=True, stdout="abc1234 fix\n")🤖 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 `@tests/test_core.py` around lines 1539 - 1543, Add explicit type hints to the helper _shell_side_effect: annotate parameters as *args: Any, **kwargs: Any and annotate the return type as MagicMock (from unittest.mock). If Any or MagicMock are not imported in the test module, add the necessary imports (from typing import Any and from unittest.mock import MagicMock) and update the function signature to: def _shell_side_effect(*args: Any, **kwargs: Any) -> MagicMock:. Keep the function body unchanged.Source: Coding guidelines
sova/core/steps/monitor_ci.py (2)
174-174:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
log.exceptionorexc_info=Truewhen logging caught exceptions.Both
except RuntimeErrorhandlers log errors without traceback context. As per coding guidelines, exceptions should be logged with stack traces.Proposed fix
except RuntimeError as exc: - log.error("step.monitor_ci.llm_failed", error=str(exc)) + log.exception("step.monitor_ci.llm_failed", error=str(exc)) return StepResult(Apply the same change at line 228:
except RuntimeError as exc: - log.error("step.monitor_ci.push_failed", error=str(exc)) + log.exception("step.monitor_ci.push_failed", error=str(exc)) return StepResult(Also applies to: 228-228
🤖 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/core/steps/monitor_ci.py` at line 174, The except RuntimeError handlers in monitor_ci.py log exceptions without tracebacks (e.g., the call log.error("step.monitor_ci.llm_failed", error=str(exc))); update these to log the full exception context by using log.exception("step.monitor_ci.llm_failed", extra={"error": str(exc)}) or log.error("step.monitor_ci.llm_failed", exc_info=True, error=str(exc)) in the handler that wraps the LLM call (reference the exception handling block around the LLM call in monitor_ci.py) and make the identical change for the second handler mentioned (the other except RuntimeError occurrence).Source: Coding guidelines
45-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImplement real
validate_outputchecks instead of unconditional pass.This gate always passes, violating the step contract. As per coding guidelines, every validate_output must check unstaged diff, staged diff, and commits ahead of base.
Proposed fix
async def validate_output(self, ctx: ExecutionContext) -> GateCheckResult: - return GateCheckResult(passed=True) + from sova.utils.shell import run + + unstaged = await run("git", "diff", "--stat", cwd=ctx.working_dir) + staged = await run("git", "diff", "--cached", "--stat", cwd=ctx.working_dir) + ahead = await run("git", "log", f"{ctx.base_branch}..HEAD", "--oneline", cwd=ctx.working_dir) + + has_changes = ( + (unstaged.success and bool(unstaged.stdout.strip())) + or (staged.success and bool(staged.stdout.strip())) + or (ahead.success and bool(ahead.stdout.strip())) + ) + if has_changes: + return GateCheckResult(passed=True) + return GateCheckResult(passed=False, reason="No unstaged diff, staged diff, or commits ahead of base")🤖 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/core/steps/monitor_ci.py` around lines 45 - 46, The current validate_output in MonitorCI unconditionally returns GateCheckResult(passed=True); replace it with real checks that follow the step contract by inspecting the ExecutionContext (use ctx) for unstaged diff, staged diff, and commits ahead of base, and return GateCheckResult(passed=...) accordingly; specifically, implement logic in validate_output to call the context methods/properties that expose unstaged_diff, staged_diff, and commits_ahead (or their equivalents), mark passed=False and include a descriptive message when any of these checks indicate a failing condition, and ensure the final GateCheckResult contains any diagnostic details needed for downstream handling.Source: Coding guidelines
sova/git/pr.py (1)
381-391:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAccount for
\n\nseparators in the cumulative budget.The budget tracking subtracts
len(section)but the final"\n\n".join(sections)adds 2 bytes per separator. With multiple failed runs, the returned string can exceedmax_log_charsby up to2 * (len(sections) - 1).Proposed fix
for check in failed_checks: + separator_cost = 2 if sections else 0 + if remaining <= separator_cost: + break + remaining -= separator_cost + run_id = _parse_run_id(check.details_url) if not run_id or run_id in seen_runs: continue🤖 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/git/pr.py` around lines 381 - 391, The code assembling sections into a final log ignores the 2-byte "\n\n" separator between sections, so the returned string can exceed max_log_chars; update the budgeting logic in the loop that builds header/output/section (variables: header, budget, output, section, sections, remaining) to account for the separator: when computing budget subtract an extra 2 bytes if sections is not empty (budget = remaining - len(header) - (2 if sections else 0)), and when updating remaining after appending a section subtract len(section) plus 2 if sections was not empty (remaining -= len(section) + (2 if sections else 0)).
🤖 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/services/agent_recovery.py`:
- Around line 152-158: The current falsy check "if not handoff:" in
agent_recovery.py incorrectly treats empty dicts as missing handoff; change the
condition to explicitly check for None (e.g., "if handoff is None:") so only a
NULL/missing handoff_json triggers the fallback returning {"has_sova_review":
True, "verdict": "revise", ...}; update the branch that references the local
variable handoff (and any use of handoff_json) to use the explicit None
comparison to preserve the later logic that handles empty dicts and computes
"approve".
---
Duplicate comments:
In `@sova/core/steps/monitor_ci.py`:
- Line 174: The except RuntimeError handlers in monitor_ci.py log exceptions
without tracebacks (e.g., the call log.error("step.monitor_ci.llm_failed",
error=str(exc))); update these to log the full exception context by using
log.exception("step.monitor_ci.llm_failed", extra={"error": str(exc)}) or
log.error("step.monitor_ci.llm_failed", exc_info=True, error=str(exc)) in the
handler that wraps the LLM call (reference the exception handling block around
the LLM call in monitor_ci.py) and make the identical change for the second
handler mentioned (the other except RuntimeError occurrence).
- Around line 45-46: The current validate_output in MonitorCI unconditionally
returns GateCheckResult(passed=True); replace it with real checks that follow
the step contract by inspecting the ExecutionContext (use ctx) for unstaged
diff, staged diff, and commits ahead of base, and return
GateCheckResult(passed=...) accordingly; specifically, implement logic in
validate_output to call the context methods/properties that expose
unstaged_diff, staged_diff, and commits_ahead (or their equivalents), mark
passed=False and include a descriptive message when any of these checks indicate
a failing condition, and ensure the final GateCheckResult contains any
diagnostic details needed for downstream handling.
In `@sova/git/pr.py`:
- Around line 381-391: The code assembling sections into a final log ignores the
2-byte "\n\n" separator between sections, so the returned string can exceed
max_log_chars; update the budgeting logic in the loop that builds
header/output/section (variables: header, budget, output, section, sections,
remaining) to account for the separator: when computing budget subtract an extra
2 bytes if sections is not empty (budget = remaining - len(header) - (2 if
sections else 0)), and when updating remaining after appending a section
subtract len(section) plus 2 if sections was not empty (remaining -=
len(section) + (2 if sections else 0)).
In `@tests/test_core.py`:
- Around line 1539-1543: Add explicit type hints to the helper
_shell_side_effect: annotate parameters as *args: Any, **kwargs: Any and
annotate the return type as MagicMock (from unittest.mock). If Any or MagicMock
are not imported in the test module, add the necessary imports (from typing
import Any and from unittest.mock import MagicMock) and update the function
signature to: def _shell_side_effect(*args: Any, **kwargs: Any) -> MagicMock:.
Keep the function body unchanged.
🪄 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: bbc59102-73af-41e1-86c0-24dfa24674c4
⛔ Files ignored due to path filters (1)
.claude/rules/architecture.mdis excluded by!.claude/**and included by none
📒 Files selected for processing (9)
sova/config/models.pysova/core/steps/monitor_ci.pysova/core/steps/validate.pysova/dashboard/services/agent_db.pysova/dashboard/services/agent_recovery.pysova/git/operations.pysova/git/pr.pytests/test_core.pytests/test_dashboard.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
sova/dashboard/services/agent_recovery.py (1)
152-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFalsy check treats empty dict differently than intended.
Line 152 uses
if not handoff:which evaluates to True for bothNoneand empty dict{}. This was flagged in past review comments but not addressed.Impact: If
handoff_json = {}, the old logic (lines 160-169) would compute verdict "approve" (no next_action, no findings), but this new check returns "revise" instead, changing semantics.Fix: Change to explicit None check:
handoff = run.handoff_json - if not handoff: + if handoff is None: return {🤖 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/services/agent_recovery.py` around lines 152 - 158, The current conditional uses a falsy check on handoff (if not handoff) which treats an empty dict the same as None and alters logic; update the check to explicitly test for None (e.g., if handoff is None) so that an empty dict proceeds to the existing verdict computation path; locate the condition around the handoff variable in the agent_recovery logic (the block returning {"has_sova_review": True, "verdict": "revise", ...}) and replace the falsy check with an explicit None comparison to preserve the prior semantics where {} is processed rather than treated as absence.
🤖 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/services/agent_db.py`:
- Around line 158-163: The current falsy checks around handoff (e.g.,
task_run.handoff_json in agent_db.py and run.handoff_json in agent_recovery.py)
treat empty dicts as “no handoff” which is ambiguous; decide whether an empty
dict is a valid payload and then make checks explicit and consistent: if {}
should be considered valid, replace `if not task_run.handoff_json` and `if not
handoff` with `if task_run.handoff_json is None` / `if run.handoff_json is None`
in agent_db.py and agent_recovery.py respectively; otherwise, add a short
comment near the `if not ...` checks documenting that an empty dict means “no
handoff” to make intent clear (ensure the persistence logic around file_handoff
and the verdict "revise" behavior remain aligned).
In `@tests/test_dashboard.py`:
- Around line 2713-2733: The current falsy check for handoff_json in
get_sova_review_verdict treats empty dicts like None, causing {} to be handled
as a null handoff; change the logic to distinguish None vs empty dict by
checking explicitly for None (e.g., use "if handoff_json is None" where the code
currently uses a falsy check), and ensure subsequent branches that read
next_action or findings handle an empty dict (no next_action -> default to
approve, empty findings -> finding_count zero). Update the conditional in
agent_recovery.py around the handoff_json handling to use the explicit None
check and keep existing behavior for actual None values.
---
Duplicate comments:
In `@sova/dashboard/services/agent_recovery.py`:
- Around line 152-158: The current conditional uses a falsy check on handoff (if
not handoff) which treats an empty dict the same as None and alters logic;
update the check to explicitly test for None (e.g., if handoff is None) so that
an empty dict proceeds to the existing verdict computation path; locate the
condition around the handoff variable in the agent_recovery logic (the block
returning {"has_sova_review": True, "verdict": "revise", ...}) and replace the
falsy check with an explicit None comparison to preserve the prior semantics
where {} is processed rather than treated as absence.
🪄 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: 2c2b13c6-7cd6-4687-a6ca-7f8faa95793f
⛔ Files ignored due to path filters (1)
.claude/rules/architecture.mdis excluded by!.claude/**and included by none
📒 Files selected for processing (9)
sova/config/models.pysova/core/steps/monitor_ci.pysova/core/steps/validate.pysova/dashboard/services/agent_db.pysova/dashboard/services/agent_recovery.pysova/git/operations.pysova/git/pr.pytests/test_core.pytests/test_dashboard.py
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
f764761 to
eddfb68
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/core/steps/monitor_ci.py`:
- Around line 204-212: The fetched CI logs (ci_logs from get_ci_failure_logs)
are being passed verbatim into _build_fix_prompt and must be sanitized first;
add a redaction/sanitization step (e.g., a helper sanitize_ci_logs or
redact_sensitive_info) immediately after ci_logs is assigned and before it's
used in _build_fix_prompt/run to strip/obfuscate secrets (API keys, tokens,
bearer strings), internal URLs, emails, SSH keys, and other PII using targeted
regexes and length-limited context excerpts; ensure you call this same sanitizer
for any other place where raw CI logs are injected into prompts (the code path
that prepares the prompt and any subsequent uses) and keep the sanitized output
in the ci_logs variable passed to _build_fix_prompt so no raw logs reach the
LLM.
- Around line 136-142: The current budget check uses ctx.is_budget_exceeded
(which excludes the case remaining_budget == 0) and allows a final paid LLM
call; change the terminal condition to treat zero as exhausted by checking
ctx.remaining_budget <= 0 (or combining both: if getattr(ctx,
"remaining_budget", None) is not None and ctx.remaining_budget <= 0) wherever
ctx.is_budget_exceeded is used (e.g. the block that returns StepResult with
"Budget exceeded..." and the other check around the fixer invocation), so the
code returns the StepResult early and never proceeds to invoke the fixer or make
the LLM call when remaining_budget is 0 or less.
In `@sova/dashboard/services/agent_db.py`:
- Around line 119-121: file_handoff["issue"] is compared raw to
task_run.issue_number (which is normalized in TaskRun), so values like "`#105`"
won't match "105"; update the matching logic (where issue_match is computed) to
normalize the handoff issue by converting to string, stripping whitespace and
any leading '#' (and handle None) before comparing to
str(task_run.issue_number), keeping the existing pr_match logic for
file_handoff["pr_number"] intact.
In `@tests/test_dashboard.py`:
- Line 2736: The test functions (e.g., test_returns_none_when_no_file) are
missing parameter type annotations for the tmp_path fixture; update each
affected test signature (including the ones at the other reported locations) to
annotate tmp_path as pathlib.Path (e.g., def
test_returns_none_when_no_file(self, tmp_path: pathlib.Path) -> None:) and
ensure pathlib is imported where needed so the added annotations resolve.
🪄 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: fb1a88af-0247-4267-9b6c-20bead170370
⛔ Files ignored due to path filters (1)
.claude/rules/architecture.mdis excluded by!.claude/**and included by none
📒 Files selected for processing (10)
sova/config/models.pysova/core/steps/monitor_ci.pysova/core/steps/validate.pysova/dashboard/services/agent_db.pysova/dashboard/services/agent_recovery.pysova/git/operations.pysova/git/pr.pytests/test_core.pytests/test_dashboard.pytests/test_git.py
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
eddfb68 to
2eb22f5
Compare
xsovad06
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
Stale: all findings addressed in subsequent commits.
|
@coderabbitai resolve |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
sova/dashboard/services/agent_db.py (2)
115-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
TaskRuntype in_apply_file_handoffsignature.Line 115 still uses
task_run: object, which violates the required signature typing rule and weakens static checks.Proposed fix
-def _apply_file_handoff(task_run: object, file_handoff: dict | None, run_id: int) -> None: +def _apply_file_handoff(task_run: "TaskRun", file_handoff: dict | None, run_id: int) -> None:As per coding guidelines,
**/*.py: Type hints are required on all Python function signatures.🤖 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/services/agent_db.py` at line 115, Change the _apply_file_handoff signature to use the concrete TaskRun type instead of object: update the parameter annotation from task_run: object to task_run: TaskRun in the function definition for _apply_file_handoff and add the appropriate import for TaskRun at the top of the module (or reference its module) so the type is resolved; ensure any internal usages remain compatible with TaskRun methods/attributes.Source: Coding guidelines
119-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize handoff issue before matching
TaskRun.issue_number.Line 119 compares raw handoff issue text to
task_run.issue_number.TaskRun.issue_numberis normalized (leading#stripped), so#105in handoff will not match105, and persistence is skipped.Proposed fix
- issue_match = file_handoff["issue"] and str(file_handoff["issue"]) == str(task_run.issue_number) - pr_match = file_handoff["pr_number"] and file_handoff["pr_number"] == task_run.pr_number + handoff_issue = str(file_handoff.get("issue", "")).lstrip("#").strip() + task_issue = str(task_run.issue_number).lstrip("#").strip() + issue_match = bool(handoff_issue) and handoff_issue == task_issue + handoff_pr = file_handoff.get("pr_number") + pr_match = handoff_pr is not None and handoff_pr == task_run.pr_number🤖 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/services/agent_db.py` around lines 119 - 121, The comparison for issue_match uses raw file_handoff["issue"] but TaskRun.issue_number is stored normalized (leading '#' removed), so normalize the handoff issue before comparison: when computing issue_match (and similar checks), coerce file_handoff["issue"] to a string, strip surrounding whitespace and a leading '#' (and possibly other non-digit prefixes if your TaskRun normalization does), then compare to str(task_run.issue_number); update the expression that defines issue_match in agent_db.py to use the normalized_handoff value so '`#105`' correctly matches '105'.tests/test_dashboard.py (1)
2736-2736:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd type hints for
tmp_pathin new test signatures.Lines 2736, 2741, and 2767 omit parameter annotations, which violates the project typing rule for Python signatures.
Proposed fix
+from pathlib import Path ... - def test_returns_none_when_no_file(self, tmp_path) -> None: + def test_returns_none_when_no_file(self, tmp_path: Path) -> None: ... - def test_reads_valid_handoff(self, tmp_path) -> None: + def test_reads_valid_handoff(self, tmp_path: Path) -> None: ... - def test_returns_none_on_corrupt_json(self, tmp_path) -> None: + def test_returns_none_on_corrupt_json(self, tmp_path: Path) -> None:As per coding guidelines,
**/*.py: Type hints are required on all Python function signatures.Also applies to: 2741-2741, 2767-2767
🤖 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 `@tests/test_dashboard.py` at line 2736, Add missing parameter type hints for the pytest tmp_path fixture on the three test functions (e.g., test_returns_none_when_no_file, the function at line 2741, and the function at line 2767) by annotating the tmp_path parameter as pathlib.Path (tmp_path: pathlib.Path); ensure pathlib is imported in the test module if not already. This will satisfy the project's requirement that all Python function signatures include type hints.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@sova/dashboard/services/agent_db.py`:
- Line 115: Change the _apply_file_handoff signature to use the concrete TaskRun
type instead of object: update the parameter annotation from task_run: object to
task_run: TaskRun in the function definition for _apply_file_handoff and add the
appropriate import for TaskRun at the top of the module (or reference its
module) so the type is resolved; ensure any internal usages remain compatible
with TaskRun methods/attributes.
- Around line 119-121: The comparison for issue_match uses raw
file_handoff["issue"] but TaskRun.issue_number is stored normalized (leading '#'
removed), so normalize the handoff issue before comparison: when computing
issue_match (and similar checks), coerce file_handoff["issue"] to a string,
strip surrounding whitespace and a leading '#' (and possibly other non-digit
prefixes if your TaskRun normalization does), then compare to
str(task_run.issue_number); update the expression that defines issue_match in
agent_db.py to use the normalized_handoff value so '`#105`' correctly matches
'105'.
In `@tests/test_dashboard.py`:
- Line 2736: Add missing parameter type hints for the pytest tmp_path fixture on
the three test functions (e.g., test_returns_none_when_no_file, the function at
line 2741, and the function at line 2767) by annotating the tmp_path parameter
as pathlib.Path (tmp_path: pathlib.Path); ensure pathlib is imported in the test
module if not already. This will satisfy the project's requirement that all
Python function signatures include type hints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a608251-c54c-48a9-be9c-727371937bbb
📒 Files selected for processing (3)
sova/dashboard/services/agent_db.pysova/dashboard/services/agent_recovery.pytests/test_dashboard.py
✅ Action performedComments resolved and changes approved. |
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
2eb22f5 to
a4a8aec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/steps/monitor_ci.py`:
- Around line 77-91: The monitor_ci step incorrectly treats a CI-fetch failure
as "no checks" and can false-pass; update step.monitor_ci to call get_ci_checks
with the GitHub user (pass ctx.github_user) and change its check of the result
to distinguish an explicit fetch failure from an empty list (e.g., treat None or
an error sentinel as a fetch failure and do not return success). Simultaneously
modify sova/git/pr.py's get_ci_checks to stop collapsing real failures into [] —
either raise on CLI/JSON errors or return an explicit status/value (e.g., None
or (success, checks)) so monitor_ci can handle failures vs. genuinely empty
check lists; ensure StepResult success is only returned for a true empty-checks
case after the grace period.
In `@sova/git/pr.py`:
- Around line 327-334: The _parse_run_id function can return run IDs that
include query strings or fragments (e.g. "12345?check_suite_focus=true"), which
breaks subsequent gh run view calls; update _parse_run_id to sanitize the
extracted segment by stripping any query (?) or fragment (#) suffix before
returning (e.g., parse the segment and split on '?' and '#' or use urllib.parse
to remove query/fragment), ensuring the returned run_id is only
digits/identifier characters.
🪄 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: 477f93da-a22c-4f00-85fc-75907d2b7a7a
⛔ Files ignored due to path filters (1)
.claude/rules/architecture.mdis excluded by!.claude/**and included by none
📒 Files selected for processing (10)
sova/config/models.pysova/core/steps/monitor_ci.pysova/core/steps/validate.pysova/dashboard/services/agent_db.pysova/dashboard/services/agent_recovery.pysova/git/operations.pysova/git/pr.pytests/test_core.pytests/test_dashboard.pytests/test_git.py
Add get_ci_failure_logs() to fetch failed-job logs via gh CLI. get_ci_checks() now returns None on fetch failures (instead of []) so callers can distinguish "no checks configured" from "unable to query GitHub". _parse_run_id() uses urlparse to strip query params. New ci.max_fix_attempts config (default 0 = disabled, capped at 3). find_pre_push_hook() extracted as public for reuse.
When CI checks fail, MonitorCIStep enters a fix loop: fetches CI logs, invokes Claude to fix and commit, validates with local pre-push hooks, pushes, then re-polls CI. Loops up to min(ci.max_fix_attempts, 3). CI logs are redacted before being sent to the LLM. Fetch failures (None from get_ci_checks) keep polling instead of false-passing.
_finalize_task_run now persists file-based handoff details to the DB when handoff_json is empty. get_sova_review_verdict falls back to verdict "revise" when a completed review run has no handoff_json. get_ci_checks None handling added to agent_recovery.
xsovad06
force-pushed
the
feat/ci-auto-recovery
branch
from
a4a8aec to
ca5d561
Compare
|
1 participant
Summary
MonitorCIStepnow enters a fix loop -- fetches CI failure logs viagh run view --log-failed, invokes Claude to fix and commit, validates with local pre-push hooks before pushing, then re-polls CI. Loops up tomin(ci.max_fix_attempts, 3)times. Setci.max_fix_attempts = 0in sova.toml to disable (pre-existing behaviour). Budget is checked before each attempt._finalize_task_runnow persists file-based handoff details to the DB whenhandoff_jsonis empty.get_sova_review_verdictfalls back to verdict "revise" when a completed review run has nohandoff_json. This fixes the missing "Address Review" button aftercommand:review-prruns.Closes #93
Test Plan
max_fix_attemptsconfig