fix(dashboard): agent tracking, handoff grid, PR state improvements#197
Open
xsovad06 wants to merge 6 commits into
Open
fix(dashboard): agent tracking, handoff grid, PR state improvements#197xsovad06 wants to merge 6 commits into
xsovad06 wants to merge 6 commits into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 a ChangesPR Review State: thread counts and REVIEW_ADDRESSED
Command/reviewer pipeline variant classification
Handoff hardening, issue resolution, and multi-handoff UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
xsovad06
commented
Jun 19, 2026
xsovad06
left a comment
xsovad06
left a comment
Owner
Author
There was a problem hiding this comment.
PR Review -- fix(dashboard): agent tracking, handoff grid, PR state improvements
7 commits, 15 files (+544/-142)
Findings
[HIGH] Correctness -- GraphQL query may exceed GitHub's query size limit with many PRs
sova/git/pr.py:254-262
get_review_thread_counts builds a single GraphQL query with one alias per PR. With 100 open PRs, if any single PR number is invalid (deleted after the list call), the entire GraphQL call fails and returns no data for any PR. Consider chunking pr_numbers into batches of ~20-30 to limit blast radius of a single failure. Not blocking but worth a follow-up.
[HIGH] Correctness -- _detect_variant_from_steps doesn't handle "command" variant
sova/dashboard/services/work_service.py:357-370
_detect_variant now returns "command" for command:* and reviewer roles, but _detect_variant_from_steps (the authoritative fallback when step history exists) has no "command" path. If a command-based run has step executions, it would be classified as "developer". Add a command variant check to _detect_variant_from_steps, or document that command runs never have step executions.
[MEDIUM] Correctness -- _resolve_issue_from_pr uses synchronous subprocess in async context
sova/dashboard/services/agent_lifecycle.py:641-658
_resolve_issue_from_pr uses blocking subprocess.run() in an async function, blocking the event loop for up to 10s on timeout. The project already has sova.utils.shell.run for async subprocess. Consider using that or asyncio.to_thread().
[MEDIUM] Consistency -- _STANDALONE_ROLES defined inside function body
sova/dashboard/services/agent_lifecycle.py:749
_STANDALONE_ROLES frozenset is reconstructed on every call. Other similar constants (_ADDRESS_REVIEW_ONLY, _RESEARCHER_ONLY) are module-level. Move it to module level.
[LOW] Code Quality -- latestReviews vs reviews field ambiguity
sova/dashboard/services/pr_service.py:152
raw.get("latestReviews") or raw.get("reviews") or None -- if latestReviews is an empty list (falsy), it falls through to reviews which may contain stale older reviews. Pick latestReviews only: raw.get("latestReviews") or None.
Verdict
Comment only -- solid work overall. The sync subprocess and _detect_variant_from_steps gap are the most actionable items but not blocking.
What's Done Well
- Defensive null handling with
(get("issue") or "")correctly fixes real crashes from explicit null values. - Batched GraphQL for thread counts is efficient with proper error handling and graceful degradation.
- Pipeline variant awareness for command runs (pulsing bar instead of 15-step pipeline) is clean UX with consistent implementation across backend and frontend.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sova/dashboard/services/work_service.py (1)
355-366: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
_detect_variant_from_stepslacks command variant handling.
_detect_variantnow returns"command"forcommand:*andreviewerroles, but_detect_variant_from_stepshas no equivalent logic. Inget_work_history(line 123-129), step-name matching runs first, so a command/reviewer run would fall through to_detect_variantonly if its steps don't match_RESEARCHER_ONLYor_ADDRESS_REVIEW_ONLY. This works by accident but the two functions have diverged in their variant detection contracts.Consider adding the role parameter to
_detect_variant_from_stepsor documenting why step-based detection is intentionally skipped for command runs.🤖 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/work_service.py` around lines 355 - 366, The function `_detect_variant_from_steps` is missing the command variant detection logic that exists in `_detect_variant`, causing inconsistency between the two functions. Add a role parameter to `_detect_variant_from_steps` and include logic to return "command" when the role is "command" or "reviewer" (matching the existing behavior in `_detect_variant`), then update the caller in `get_work_history` to pass the role parameter when invoking `_detect_variant_from_steps` so that command variant detection works consistently through both detection paths.
🤖 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/routers/handoff.py`:
- Around line 69-70: The generator expression that filters actions is using
incorrect logic by combining field values with `or` before comparison, which
only tests one field against `req.action_id` and skips other fields if the first
one exists. Fix this by changing the conditional to perform individual equality
checks for each candidate field (id, command, and label) combined with `or`
operators, so that `req.action_id` is matched against all three fields
independently rather than just the first truthy field value. Apply this fix in
both locations where this pattern appears in the handoff.py file.
In `@sova/dashboard/services/agent_lifecycle.py`:
- Around line 634-653: The subprocess.run call in the _resolve_issue_from_pr
function should explicitly include check=False to silence the PLW1510 warning,
since the code intentionally handles non-zero return codes. Additionally, add an
input validation check at the start of the _resolve_issue_from_pr function to
ensure pr_number is numeric (either by converting it to int or adding an
explicit assertion), which provides defense-in-depth validation at the function
boundary and clarifies the security posture to static analysis tools.
- Around line 743-751: The constant _STANDALONE_ROLES is defined inside a
function, causing it to be recreated on every function call. Move the
_STANDALONE_ROLES constant definition from inside the function to module level,
placing it alongside other similar module-level constants defined at the top of
the file in sova/dashboard/services/agent_lifecycle.py. Then update the function
to reference this module-level constant instead of redefining it locally.
In `@sova/dashboard/templates/agents.html`:
- Around line 907-908: The `issueArg` variable is being HTML-escaped and then
inserted into inline JavaScript event handlers (onclick), which is an XSS
vulnerability because HTML escaping is not the correct escaping context for
JavaScript strings. Instead of using inline handlers with HTML-escaped values,
refactor to attach event listeners via DOM methods or use data attributes to
pass the issue value safely, then access it from within the event handler using
JavaScript string-safe techniques. This same issue appears in the `issueArg`
variable usage throughout the handlers at lines 915-916 and 940, so apply the
fix consistently across all occurrences where `issueArg` is interpolated into
onclick or similar inline handlers.
In `@sova/git/pr.py`:
- Around line 261-263: The GraphQL query in the PR review threads check is
fetching only the first 100 review threads with reviewThreads(first:100), but
counts resolved threads only from those returned nodes. This causes incorrect
classification of PRs with more than 100 review threads. Either implement
pagination by adding pageInfo with hasNextPage and endCursor fields to the
query, then loop through all pages using the after cursor parameter to fetch
remaining threads, or increase the hardcoded limit from 100 to a higher value
appropriate for your repository scale. Ensure that the resolved thread count is
compared against the actual totalCount of all threads, not just the fetched
subset.
---
Outside diff comments:
In `@sova/dashboard/services/work_service.py`:
- Around line 355-366: The function `_detect_variant_from_steps` is missing the
command variant detection logic that exists in `_detect_variant`, causing
inconsistency between the two functions. Add a role parameter to
`_detect_variant_from_steps` and include logic to return "command" when the role
is "command" or "reviewer" (matching the existing behavior in
`_detect_variant`), then update the caller in `get_work_history` to pass the
role parameter when invoking `_detect_variant_from_steps` so that command
variant detection works consistently through both detection paths.
🪄 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: 11d160d0-2fbc-481e-9208-937a79477487
⛔ Files ignored due to path filters (3)
.claude/commands/update-roadmap.mdis excluded by!.claude/**and included by none.githooks/pre-pushis excluded by none and included by nonedocs/roadmap.htmlis excluded by!docs/**and included by none
📒 Files selected for processing (12)
sova/dashboard/routers/handoff.pysova/dashboard/services/agent_lifecycle.pysova/dashboard/services/handoff_service.pysova/dashboard/services/pr_service.pysova/dashboard/services/work_service.pysova/dashboard/static/app.jssova/dashboard/templates/agents.htmlsova/dashboard/templates/dashboard.htmlsova/dashboard/templates/run_detail.htmlsova/git/pr.pytests/test_dashboard.pytests/test_pr_service.py
xsovad06
added a commit
that referenced
this pull request
Jun 19, 2026
Fix action_id matching to check all candidate fields independently, convert _resolve_issue_from_pr to async, move _STANDALONE_ROLES to module level, use escapeJsStr for onclick handlers, add command variant to _detect_variant_from_steps, and use latestReviews without fallback.
xsovad06
force-pushed
the
fix/dashboard-agent-tracking
branch
from
b1f5918 to
6e13bdd
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
All review findings addressed in commit 6e13bdd:
Status: ALL_RESOLVED |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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_lifecycle.py`:
- Around line 445-447: The unconditional int(raw_pr) conversion on line 447 can
raise an exception if raw_pr contains a non-numeric value, causing unhandled 500
errors. Wrap the int conversion in a try-except block to catch ValueError
exceptions and set pr_number to None or a default value on failure, returning an
appropriate error response. Apply the same defensive handling to the duplicate
unsafe coercion patterns at lines 451-453 and the helper function around lines
635-640, ensuring all PR number parsing validates input before conversion and
fails gracefully with meaningful error messages.
In `@sova/dashboard/services/handoff_service.py`:
- Line 152: The code at line 152 and line 218 attempts to call string methods
`.lstrip()` and `.strip()` on issue values from JSON that may not be strings
(legacy values could be integers like 123), which will raise an AttributeError.
Wrap the issue value retrieval with `str()` to coerce it to a string before
calling string methods. Change `(ldata.get("issue") or "")` to
`str(ldata.get("issue") or "")` at both locations to ensure the value is always
a string before `.lstrip("#").strip()` is invoked.
In `@sova/dashboard/services/pr_service.py`:
- Around line 217-221: Replace the broad `except Exception:` clause with a tuple
of specific expected exceptions that the `get_review_thread_counts` function
call could raise (such as OSError, json.JSONDecodeError, or other
network/API-related exceptions). This prevents masking unexpected bugs while
still gracefully handling known failure modes by falling back to empty
thread_counts.
In `@sova/dashboard/static/app.js`:
- Around line 792-796: The variable cmdName is being directly concatenated into
an HTML string in the label without escaping, creating an XSS vulnerability. In
the code where cmdName is created from the role parameter and then inserted into
the label div element, use a proper HTML escaping function to escape cmdName
before concatenating it into the HTML string. This ensures that any special HTML
characters in cmdName are properly encoded and cannot be interpreted as
executable code.
In `@sova/git/pr.py`:
- Line 257: The line unpacking repo.split("/", 1) into owner and name assumes
the repo string contains at least one "/" separator. If repo is malformed (e.g.,
just "reponame" without a slash), this will raise a ValueError during unpacking.
Add validation to check that the split result contains exactly 2 elements before
unpacking, and raise a descriptive ValueError if the repo format is invalid.
This validation should occur before the unpacking assignment of owner, name to
handle potentially misconfigured cfg.github_repo values passed from
list_open_prs_with_state.
- Around line 258-263: Replace the manual for loop with aliases initialization
and append calls with a list comprehension. Instead of initializing an empty
aliases list and looping through pr_numbers to append the f-string, move the
f-string expression (that combines "pr{pr_num}: pullRequest(number:{pr_num}) {{
reviewThreads(first:100) {{ totalCount nodes {{ isResolved }} }} }}") directly
into a list comprehension that iterates over pr_numbers. This will create the
aliases list in a more concise and Pythonic way.
🪄 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: c791d118-8878-48e7-b1f2-02582307a4df
⛔ Files ignored due to path filters (3)
.claude/commands/update-roadmap.mdis excluded by!.claude/**and included by none.githooks/pre-pushis excluded by none and included by nonedocs/roadmap.htmlis excluded by!docs/**and included by none
📒 Files selected for processing (12)
sova/dashboard/routers/handoff.pysova/dashboard/services/agent_lifecycle.pysova/dashboard/services/handoff_service.pysova/dashboard/services/pr_service.pysova/dashboard/services/work_service.pysova/dashboard/static/app.jssova/dashboard/templates/agents.htmlsova/dashboard/templates/dashboard.htmlsova/dashboard/templates/run_detail.htmlsova/git/pr.pytests/test_dashboard.pytests/test_pr_service.py
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
xsovad06
force-pushed
the
fix/dashboard-agent-tracking
branch
from
11fa205 to
7fe03b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
sova/dashboard/services/agent_lifecycle.py (1)
636-663:⚠️ Potential issue | 🟡 Minor | 💤 Low valueThe
int()call at line 643 is outside the try block.If
_resolve_issue_from_pris ever called with a non-numeric string,str(int(pr_number))raisesValueErrorbefore entering the try block, bypassing the exception handler and propagating up uncaught.Current callers validate input first (line 447-451), so this is not immediately exploitable. However, the function signature accepts
int | str, implying it should handle string input gracefully.Proposed fix
async def _resolve_issue_from_pr(pr_number: int | str, project_dir: Path) -> str: """Extract a linked issue number from a PR body via gh CLI (best-effort).""" import re + try: + pr_str = str(int(pr_number)) + except (ValueError, TypeError): + return "" try: - pr_str = str(int(pr_number)) result = await run_shell(🤖 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_lifecycle.py` around lines 636 - 663, The int() conversion of pr_number in the _resolve_issue_from_pr function is currently outside the try block, which means if a non-numeric string is passed, the ValueError will not be caught by the exception handler. Move the line pr_str = str(int(pr_number)) inside the try block so that any ValueError from invalid input is properly handled and logged, allowing the function to return an empty string gracefully rather than propagating an unhandled exception up the call stack.
🤖 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/routers/handoff.py`:
- Around line 96-103: The else branch handling the "pr-review-state" case does
not wrap the issue value with str() when extracting it from the handoff
dictionary, creating a type mismatch for the invalidate_synthesis_cache function
call. To fix this, apply the str() wrapper to the issue assignment in the else
block (where issue is assigned from handoff.get("issue")) to ensure the value is
always a string, matching the pattern used in the if branch for the
clear_handoff case and ensuring consistent cache key types when
invalidate_synthesis_cache is called.
- Around line 105-110: The issue parameter passed to the `start_agent` method
call needs to be wrapped with `str()` to ensure type safety, as the method
expects a string but `exec_params.get("issue")` could return a non-string truthy
value. Apply the same defensive `str()` wrapper pattern that is already used
elsewhere in this file (at lines 62 and 64) by wrapping the expression
`exec_params.get("issue") or ""` with `str()` to guarantee the parameter is
always a string before being passed to `start_agent`.
In `@sova/dashboard/templates/agents.html`:
- Around line 252-257: The code is using escapeHtml() for values that are placed
inside JavaScript string literals within onclick handlers (such as in the
runCommand and quickStartRole function calls), but HTML escaping is insufficient
for JavaScript string contexts and can be broken by crafted input. Replace all
instances of escapeHtml(t.issue) with a proper JavaScript string escaping
function (such as escapeJsString or similar) when t.issue appears inside
JavaScript string literals in the onclick attributes throughout the template,
including the handlers in runCommand, quickStartRole, and any other similar
patterns at lines 291-293 and 298-300.
- Around line 251-253: The button command in the condition checking for smartAct
values of 'integrate' or 'ship-pr' always calls runCommand with the hardcoded
command 'integrate-pr', regardless of which action is being executed. Fix this
by using a conditional expression to determine the correct command name to pass
to runCommand: use 'integrate-pr' when smartAct is 'integrate' and 'ship-pr'
when smartAct is 'ship-pr', ensuring the correct workflow is triggered based on
the actual smartAct value.
In `@sova/git/pr.py`:
- Around line 280-286: The code assumes that data.get("data", {}) always returns
a dictionary, but a valid GraphQL error response can set the "data" field to
null, which causes the subsequent .get() call on repo_data to raise an
AttributeError. Add a guard check to verify that repo_data is actually a
dictionary before attempting to access its nested properties in the for loop
that iterates over pr_numbers. Ensure the function returns an empty dict {} on
failure as intended, by either checking if repo_data is a valid dict or by
catching the case where the GraphQL payload has null values.
In `@tests/test_pr_service.py`:
- Around line 395-407: The test only covers the case where GraphQL returns an
empty repository object, but misses the valid-JSON error case where data is null
with an errors array. Add a new test case (or extend the existing test in
test_handles_missing_pr_data) that creates a graphql_response with {"data":
null, "errors": [...]} as the mocked response, follows the same setup and
mocking pattern, invokes get_review_thread_counts with the same parameters, and
asserts the function returns (0, 0) for the PR number to verify it properly
falls back when encountering this distinct GraphQL error shape.
---
Duplicate comments:
In `@sova/dashboard/services/agent_lifecycle.py`:
- Around line 636-663: The int() conversion of pr_number in the
_resolve_issue_from_pr function is currently outside the try block, which means
if a non-numeric string is passed, the ValueError will not be caught by the
exception handler. Move the line pr_str = str(int(pr_number)) inside the try
block so that any ValueError from invalid input is properly handled and logged,
allowing the function to return an empty string gracefully rather than
propagating an unhandled exception up the call stack.
🪄 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: c58618b5-9d2e-431b-bbd1-7a965a8620eb
⛔ Files ignored due to path filters (3)
.claude/commands/update-roadmap.mdis excluded by!.claude/**and included by none.githooks/pre-pushis excluded by none and included by nonedocs/roadmap.htmlis excluded by!docs/**and included by none
📒 Files selected for processing (12)
sova/dashboard/routers/handoff.pysova/dashboard/services/agent_lifecycle.pysova/dashboard/services/handoff_service.pysova/dashboard/services/pr_service.pysova/dashboard/services/work_service.pysova/dashboard/static/app.jssova/dashboard/templates/agents.htmlsova/dashboard/templates/dashboard.htmlsova/dashboard/templates/run_detail.htmlsova/git/pr.pytests/test_dashboard.pytests/test_pr_service.py
…ommand Add invariant check scripts to the pre-push hook alongside existing lint and test checks. Add update-roadmap command for maintaining roadmap HTML from GitHub Issue states. Fix Mermaid arrow syntax escaping in roadmap instruction comments.
Add GraphQL batch-fetch for review thread counts (total vs resolved) to detect when a PR has addressed review feedback but GitHub's reviewDecision hasn't updated. Introduce REVIEW_ADDRESSED computed state for PRs where threads are resolved but no new approval exists. Use latestReviews to detect active CHANGES_REQUESTED even when reviewDecision is stale. Includes tests for all new state transitions and review thread count fetching.
Normalize issue values with str() before string operations in handoff service (archive, clear) and router (execute, cache key). Match actions by id, command, or label for flexibility. Prevents TypeErrors when issue is stored as integer in handoff JSON.
Fix step progress for command-based runs (integrate-pr, ship-pr) and standalone roles (reviewer) to show single-step pulsing bar instead of 15-step developer pipeline. Resolve linked issue from PR body when commands are started with only a PR number. Pass pipeline_variant to all templates for correct progress rendering. Add variant detection for commands in work_service. Includes tests for issue resolution, variant detection, and step progress.
…dering Overhaul the agents page frontend: Running agent detection: fix field name mismatch (issue vs issue_number) that made _runningIssues always empty. Show role-specific labels (Integrating, Reviewing, etc.) when agents run. Smart action buttons: derive primary action from PR state instead of static issue-state mapping. Show Integrate when PR is approved, Address Review when changes requested, Ship PR when approved with green CI. Each action calls the correct command. Context-aware dropdown with Stop Agent, Address Review, Integrate PR, and Ship PR. Handoff grid: replace single-handoff panel with responsive grid showing all awaiting handoffs simultaneously. Per-handoff dismiss. Deduplicate notification toasts via localStorage. Pipeline rendering: use pipeline_variant to select correct step names and counts per pipeline type (developer/address_review/researcher/ command). Fix step labels to match actual pipeline definitions.
xsovad06
force-pushed
the
fix/dashboard-agent-tracking
branch
from
7fe03b0 to
1f07638
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sova/dashboard/services/handoff_service.py (1)
177-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize archive filename components before
copy2.
h_issuecomes from handoff JSON and is embedded inarchive_name; values containing/or\can turn the archive filename into a nested path, causing archive/clear to fail or write outside the intended file name. Normalize both dynamic filename parts before building the destination.Proposed fix
- source = data.get("source", "unknown") - h_issue = data.get("issue") or "" + source = str(data.get("source") or "unknown").replace("/", "_").replace("\\", "_") + h_issue = str(data.get("issue") or "").replace("/", "_").replace("\\", "_") suffix = f"_{h_issue}" if h_issue else "" stem = hf.stem archive_name = f"{ts}_{source}{suffix}_{stem}.json"🤖 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/handoff_service.py` around lines 177 - 182, The filename components `h_issue` and `source` are derived from handoff JSON data and embedded directly into the `archive_name` without sanitization. If these values contain path separators like `/` or `\`, they can create unintended nested paths, causing the `shutil.copy2` operation to fail or write outside the intended directory. Sanitize both `h_issue` and `source` variables (and optionally `stem` from the hf object) by removing or replacing any forward slashes, backslashes, and other problematic filesystem characters before constructing the `archive_name` string.
♻️ Duplicate comments (5)
sova/dashboard/services/agent_lifecycle.py (1)
639-663:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep invalid PR strings inside the best-effort path.
Line 643 can raise before the
try, even though this helper acceptsint | strand otherwise returns""on failures. Move the coercion into the handled block. This is the same invalid-PR helper failure mode flagged earlier.Proposed fix
- pr_str = str(int(pr_number)) try: + pr_str = str(int(pr_number)) result = await run_shell( "gh", "pr", @@ - except Exception: + except (TypeError, ValueError): + return "" + except Exception: log.debug("resolve_issue_from_pr.failed", pr=pr_number, 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_lifecycle.py` around lines 639 - 663, The pr_str conversion using str(int(pr_number)) on line 643 can raise an exception before entering the try block, which violates the best-effort pattern where all failures should return an empty string. Move the pr_str = str(int(pr_number)) line inside the try block in the _resolve_issue_from_pr function so that any conversion errors are caught by the existing exception handler and the function properly returns "" on invalid PR inputs.sova/dashboard/templates/agents.html (2)
252-252:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrong escaping context for
t.issuein onclick handlers - XSS risk.Lines 252 and 260 place
t.issueinside JavaScript string literals within onclick attributes but useescapeHtml(). HTML-context escaping is insufficient for JavaScript string context and can be bypassed by crafted input liketest' + alert(1) + '. UseescapeJsStr(String(t.issue))instead.Proposed fix
} else if (smartAct === 'integrate') { - actionBtn = '<button onclick="runCommand(\'integrate-pr\', {issue: \'' + escapeHtml(t.issue) + '\', pr: ' + (prNum || 0) + '})" ' + + actionBtn = '<button onclick="runCommand(\'integrate-pr\', {issue: \'' + escapeJsStr(String(t.issue)) + '\', pr: ' + (prNum || 0) + '})" ' + 'class="text-xs px-2.5 py-1 rounded transition-colors whitespace-nowrap ' + action.cls + '">' + escapeHtml(action.text) + '</button>'; } else if (smartAct === 'ship-pr') { actionBtn = '<button onclick="runCommand(\'ship-pr\', {pr: ' + (prNum || 0) + '})" ' + 'class="text-xs px-2.5 py-1 rounded transition-colors whitespace-nowrap ' + action.cls + '">' + escapeHtml(action.text) + '</button>'; } else if (smartAct === 'address-review') { - actionBtn = '<button onclick="quickStartRole(\'' + escapeHtml(t.issue) + '\', \'developer\', ' + (prNum || 0) + ')" ' + + actionBtn = '<button onclick="quickStartRole(\'' + escapeJsStr(String(t.issue)) + '\', \'developer\', ' + (prNum || 0) + ')" ' + 'class="text-xs px-2.5 py-1 rounded transition-colors whitespace-nowrap ' + action.cls + '">' + escapeHtml(action.text) + '</button>';Also applies to: 260-260
🤖 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/templates/agents.html` at line 252, The onclick event handler at line 252 uses escapeHtml() to escape t.issue, but this is insufficient for JavaScript string literal context and leaves the code vulnerable to XSS attacks through input like `test' + alert(1) + '`. Replace escapeHtml(t.issue) with escapeJsStr(String(t.issue)) at both line 252 and line 260 where t.issue is embedded within JavaScript string literals inside the onclick attribute to provide proper JavaScript-context escaping.
294-305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrong escaping context for
t.issuein dropdown menu onclick handlers - XSS risk.Lines 295, 296, 297, 298, 302, and 304 place
t.issueinside JavaScript string literals within onclick attributes but useescapeHtml(). This is the wrong escaping context and allows script injection. Replace all instances withescapeJsStr(String(t.issue)).Proposed fix
(isRunning - ? '<button onclick="stopAgent(_runningAgents[\'' + escapeHtml(t.issue) + '\'].run_id)" class="w-full text-left px-3 py-1.5 text-xs text-accent-red hover:bg-surface-hover transition-colors">Stop Agent</button>' - : '<button onclick="quickStart(\'' + escapeHtml(t.issue) + '\', true)" class="w-full text-left px-3 py-1.5 text-xs text-gray-300 hover:bg-surface-hover transition-colors">Start with --force</button>' + - '<button onclick="quickStartRole(\'' + escapeHtml(t.issue) + '\', \'reviewer\')" class="w-full text-left px-3 py-1.5 text-xs text-gray-300 hover:bg-surface-hover transition-colors">Start as Reviewer</button>' + - '<button onclick="quickStartRole(\'' + escapeHtml(t.issue) + '\', \'triage\')" class="w-full text-left px-3 py-1.5 text-xs text-gray-300 hover:bg-surface-hover transition-colors">Start as Triage</button>') + + ? '<button onclick="stopAgent(_runningAgents[\'' + escapeJsStr(String(t.issue)) + '\'].run_id)" class="w-full text-left px-3 py-1.5 text-xs text-accent-red hover:bg-surface-hover transition-colors">Stop Agent</button>' + : '<button onclick="quickStart(\'' + escapeJsStr(String(t.issue)) + '\', true)" class="w-full text-left px-3 py-1.5 text-xs text-gray-300 hover:bg-surface-hover transition-colors">Start with --force</button>' + + '<button onclick="quickStartRole(\'' + escapeJsStr(String(t.issue)) + '\', \'reviewer\')" class="w-full text-left px-3 py-1.5 text-xs text-gray-300 hover:bg-surface-hover transition-colors">Start as Reviewer</button>' + + '<button onclick="quickStartRole(\'' + escapeJsStr(String(t.issue)) + '\', \'triage\')" class="w-full text-left px-3 py-1.5 text-xs text-gray-300 hover:bg-surface-hover transition-colors">Start as Triage</button>') + (prNum && !isRunning ? '<hr class="border-gray-700/50 my-1">' + (!linkedPr || linkedPr.computed_state === 'changes_requested' - ? '<button onclick="quickStartRole(\'' + escapeHtml(t.issue) + '\', \'developer\', ' + prNum + ')" class="w-full text-left px-3 py-1.5 text-xs text-accent-yellow hover:bg-surface-hover transition-colors">Address Review (PR #' + prNum + ')</button>' + ? '<button onclick="quickStartRole(\'' + escapeJsStr(String(t.issue)) + '\', \'developer\', ' + prNum + ')" class="w-full text-left px-3 py-1.5 text-xs text-accent-yellow hover:bg-surface-hover transition-colors">Address Review (PR #' + prNum + ')</button>' : '') + - '<button onclick="runCommand(\'integrate-pr\', {issue: \'' + escapeHtml(t.issue) + '\', pr: ' + prNum + '})" class="w-full text-left px-3 py-1.5 text-xs text-accent-green hover:bg-surface-hover transition-colors">Integrate PR #' + prNum + '</button>' + + '<button onclick="runCommand(\'integrate-pr\', {issue: \'' + escapeJsStr(String(t.issue)) + '\', pr: ' + prNum + '})" class="w-full text-left px-3 py-1.5 text-xs text-accent-green hover:bg-surface-hover transition-colors">Integrate PR #' + prNum + '</button>' + '<button onclick="runCommand(\'ship-pr\', {pr: ' + prNum + '})" class="w-full text-left px-3 py-1.5 text-xs text-gray-300 hover:bg-surface-hover transition-colors">Ship PR #' + prNum + '</button>'🤖 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/templates/agents.html` around lines 294 - 305, The onclick handlers for the agent control buttons (stopAgent, quickStart, quickStartRole, runCommand) are using escapeHtml(t.issue) to escape the issue identifier within JavaScript string literals, but this is the wrong escaping context and creates an XSS vulnerability. Replace all six instances of escapeHtml(t.issue) with escapeJsStr(String(t.issue)) throughout the dropdown menu markup to properly escape the issue identifier for JavaScript string literal context.tests/test_pr_service.py (1)
395-407: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd test coverage for GraphQL
data: nullerror responses.Current tests miss the valid GraphQL error shape
{"data": null, "errors": [...]}, which is a distinct failure mode from malformed JSON and missing PR aliases. This test gap leaves the AttributeError bug insova/git/pr.py:280uncaught.Proposed test addition
async def test_handles_missing_pr_data(self, monkeypatch) -> None: from sova.git.pr import get_review_thread_counts graphql_response = {"data": {"repository": {}}} mock_run = AsyncMock() mock_run.return_value.success = True mock_run.return_value.stdout = json.dumps(graphql_response) monkeypatch.setattr("sova.git.pr.run", mock_run) monkeypatch.setattr("sova.git.pr.resolve_gh_env", AsyncMock(return_value=None)) result = await get_review_thread_counts([10], repo="owner/repo") assert result[10] == (0, 0) + + `@pytest.mark.asyncio` + async def test_returns_empty_on_data_null(self, monkeypatch) -> None: + from sova.git.pr import get_review_thread_counts + + graphql_response = {"data": None, "errors": [{"message": "Internal error"}]} + mock_run = AsyncMock() + mock_run.return_value.success = True + mock_run.return_value.stdout = json.dumps(graphql_response) + monkeypatch.setattr("sova.git.pr.run", mock_run) + monkeypatch.setattr("sova.git.pr.resolve_gh_env", AsyncMock(return_value=None)) + + result = await get_review_thread_counts([10], repo="owner/repo") + assert result == {}🤖 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_pr_service.py` around lines 395 - 407, Add a new test method to the test class that specifically covers the GraphQL error response shape where data is null with an errors array present, similar to the existing test_handles_missing_pr_data method. This test should mock the get_review_thread_counts function call with a graphql_response containing {"data": null, "errors": [...]} to verify that the code in sova/git/pr.py around line 280 properly handles this distinct GraphQL error case and does not throw an AttributeError when accessing attributes on a null data object.sova/git/pr.py (1)
280-286:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against GraphQL
data: nullresponses.When GraphQL returns a valid JSON error response like
{"data": null, "errors": [...]}, line 280'sdata.get("data", {})returnsNone(not the default{}), causing the chained.get("repository", {})to raiseAttributeError. This violates the function's contract to gracefully return{}on any failure.Proposed fix
counts: dict[int, tuple[int, int]] = {} -repo_data = data.get("data", {}).get("repository", {}) +data_root = data.get("data") +if not isinstance(data_root, dict): + log.warning("git.review_threads.malformed_payload") + return {} +repo_data = data_root.get("repository") or {} for pr_num in pr_numbers:🤖 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 280 - 286, The issue is that when GraphQL returns a null data field like {"data": null, "errors": [...]}, the expression data.get("data", {}) returns None instead of the default empty dict, causing the subsequent .get("repository", {}) call to fail with an AttributeError since None has no get method. To fix this, guard against the None case by ensuring repo_data is always a dict before accessing it. Use the pattern (data.get("data") or {}).get("repository", {}) to safely handle null responses and maintain the function's contract to gracefully return empty results on any failure.
🤖 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/routers/handoff.py`:
- Around line 68-70: The set literal construction in the next() call that checks
`req.action_id in {a.get("id"), a.get("command"), a.get("label")}` will crash if
any of these JSON-derived fields contain unhashable values (like dicts or lists)
instead of gracefully handling the mismatch. Replace the set literal with a
tuple using the same field references so the membership test maintains the same
matching behavior without raising an exception on unhashable types. Apply this
same fix to the similar pattern around lines 83-85.
- Around line 96-98: The current code converts a missing issue to None and then
calls clear_handoff(issue=None), which performs a bulk clear of all handoff
files and can inadvertently delete unrelated issue-scoped handoffs. Instead of
passing issue=None to clear_handoff, add a conditional check to skip the clear
operation entirely when issue is None, or if clearing legacy handoffs is
necessary, use a separate method in handoff_service that clears by handoff
identity (the specific handoff being processed) rather than by issue. This
ensures that only the current legacy handoff is cleared, not all handoffs in the
system.
In `@sova/dashboard/services/agent_lifecycle.py`:
- Around line 453-458: The issue is that when JSON passes "issue": null, the
str() conversion on line 453 creates the literal string "None" instead of
treating it as an omitted issue value. The code should check if the value from
safe_args.get("issue") is None before converting it to a string, and treat None
the same as an empty or omitted value. Modify the logic to explicitly handle
None (or falsy values) separately from the string conversion, so that null
values skip the string conversion entirely and allow the subsequent PR
resolution logic on line 454 to execute properly.
In `@sova/dashboard/services/work_service.py`:
- Line 32: The condition at line 141 that gates access to total_steps_possible
only checks if r.role is in _PIPELINE_ROLES, but command-prefixed runs use
dynamic roles that may not be explicitly listed in _PIPELINE_ROLES. Update the
role check at line 141 to also accept roles that are associated with
command-prefixed history rows, allowing the command length value added at line
32 to be accessible regardless of the specific command role used. This may
require modifying the conditional logic to detect command-prefixed roles
separately or expanding the role matching criteria.
---
Outside diff comments:
In `@sova/dashboard/services/handoff_service.py`:
- Around line 177-182: The filename components `h_issue` and `source` are
derived from handoff JSON data and embedded directly into the `archive_name`
without sanitization. If these values contain path separators like `/` or `\`,
they can create unintended nested paths, causing the `shutil.copy2` operation to
fail or write outside the intended directory. Sanitize both `h_issue` and
`source` variables (and optionally `stem` from the hf object) by removing or
replacing any forward slashes, backslashes, and other problematic filesystem
characters before constructing the `archive_name` string.
---
Duplicate comments:
In `@sova/dashboard/services/agent_lifecycle.py`:
- Around line 639-663: The pr_str conversion using str(int(pr_number)) on line
643 can raise an exception before entering the try block, which violates the
best-effort pattern where all failures should return an empty string. Move the
pr_str = str(int(pr_number)) line inside the try block in the
_resolve_issue_from_pr function so that any conversion errors are caught by the
existing exception handler and the function properly returns "" on invalid PR
inputs.
In `@sova/dashboard/templates/agents.html`:
- Line 252: The onclick event handler at line 252 uses escapeHtml() to escape
t.issue, but this is insufficient for JavaScript string literal context and
leaves the code vulnerable to XSS attacks through input like `test' + alert(1) +
'`. Replace escapeHtml(t.issue) with escapeJsStr(String(t.issue)) at both line
252 and line 260 where t.issue is embedded within JavaScript string literals
inside the onclick attribute to provide proper JavaScript-context escaping.
- Around line 294-305: The onclick handlers for the agent control buttons
(stopAgent, quickStart, quickStartRole, runCommand) are using
escapeHtml(t.issue) to escape the issue identifier within JavaScript string
literals, but this is the wrong escaping context and creates an XSS
vulnerability. Replace all six instances of escapeHtml(t.issue) with
escapeJsStr(String(t.issue)) throughout the dropdown menu markup to properly
escape the issue identifier for JavaScript string literal context.
In `@sova/git/pr.py`:
- Around line 280-286: The issue is that when GraphQL returns a null data field
like {"data": null, "errors": [...]}, the expression data.get("data", {})
returns None instead of the default empty dict, causing the subsequent
.get("repository", {}) call to fail with an AttributeError since None has no get
method. To fix this, guard against the None case by ensuring repo_data is always
a dict before accessing it. Use the pattern (data.get("data") or
{}).get("repository", {}) to safely handle null responses and maintain the
function's contract to gracefully return empty results on any failure.
In `@tests/test_pr_service.py`:
- Around line 395-407: Add a new test method to the test class that specifically
covers the GraphQL error response shape where data is null with an errors array
present, similar to the existing test_handles_missing_pr_data method. This test
should mock the get_review_thread_counts function call with a graphql_response
containing {"data": null, "errors": [...]} to verify that the code in
sova/git/pr.py around line 280 properly handles this distinct GraphQL error case
and does not throw an AttributeError when accessing attributes on a null data
object.
🪄 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: e05f2246-4c27-435d-b0e9-573fc2f18586
⛔ Files ignored due to path filters (3)
.claude/commands/update-roadmap.mdis excluded by!.claude/**and included by none.githooks/pre-pushis excluded by none and included by nonedocs/roadmap.htmlis excluded by!docs/**and included by none
📒 Files selected for processing (12)
sova/dashboard/routers/handoff.pysova/dashboard/services/agent_lifecycle.pysova/dashboard/services/handoff_service.pysova/dashboard/services/pr_service.pysova/dashboard/services/work_service.pysova/dashboard/static/app.jssova/dashboard/templates/agents.htmlsova/dashboard/templates/dashboard.htmlsova/dashboard/templates/run_detail.htmlsova/git/pr.pytests/test_dashboard.pytests/test_pr_service.py
Use escapeJsStr instead of escapeHtml for t.issue in onclick handlers (correct JS string context escaping). Guard GraphQL data:null in get_review_thread_counts to prevent AttributeError. Add regression test for data:null GraphQL response.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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
Fixes multiple inconsistencies in the dashboard's agentic state display:
_runningIssueswas always empty due to field name mismatch (issuevsissue_number), so task cards never showed "Running" stateChanges
pipeline_variant: "command"with 1-step progressstr(), match actions by id/command/labelTesting
_resolve_issue_from_pr(6 cases),get_review_thread_counts(5 cases),_detect_variant/_detect_variant_from_stepscommand role detection (8 cases)make checkpasses (lint + format + tests)Checklist