feat(supervisor): LLM planning step for resource-aware action dispatch (#602) - #625
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe supervisor gains optional LLM planning. It gathers operational context, validates structured plans, filters unapproved actions, stores reasoning and deferred actions, and displays them in the dashboard. Deterministic behavior remains when planning is disabled or unavailable. ChangesLLM planning flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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/supervisor/planner.py`:
- Around line 227-273: Update _get_open_prs so a communicate timeout terminates
the still-running gh subprocess, then awaits proc.communicate() to reap it
before returning the unavailable response; preserve normal output handling and
exception behavior. Add a regression test that simulates a timeout and verifies
the process is terminated and reaped.
- Around line 41-49: Enable the SPAWN_REBASE planner contract end to end: in
sova/supervisor/planner.py lines 41-49, add spawn_rebase to _VALID_ACTIONS and
the prompt’s listed actions; in sova/supervisor/progression.py lines 247-271,
preserve approved spawn_rebase decisions through the existing action and issue
validation; in tests/test_supervisor_planner.py lines 111-114, update the
expected action set and add coverage confirming an approved rebase remains
actionable.
- Around line 159-171: Update _assemble_context and the external-data helpers it
uses to screen PR titles and failure messages for prompt-injection content and
isolate them from executable instructions before inclusion in the LLM prompt.
Preserve deterministic action progression so filtered or adversarial context
cannot suppress eligible actions, and add adversarial tests covering injected PR
titles and task failure messages; review the change against relevant OWASP
risks.
- Around line 320-323: Replace direct use of TaskRun.error_message in the
recent-failures prompt with a bounded, redacted failure category that excludes
credentials, URLs, user data, and source fragments. Preserve issue number and
role while ensuring only sanitized categories reach _call_llm. Add a test
verifying sensitive error text is absent from the prompt passed to _call_llm,
and check the change against relevant OWASP injection and
sensitive-data-exposure risks.
🪄 Autofix
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: 9149d0bc-8df6-437c-b483-59640ff662e3
⛔ Files ignored due to path filters (2)
.claude/rules/architecture.mdis excluded by!.claude/**and included by nonedocs/specs/issue-602-llm-planning.mdis excluded by!docs/**and included by none
📒 Files selected for processing (10)
sova/config/models.pysova/dashboard/routers/supervisor.pysova/dashboard/services/supervisor_service.pysova/dashboard/settings_meta.pysova/dashboard/templates/supervisor.htmlsova/supervisor/daemon.pysova/supervisor/planner.pysova/supervisor/progression.pytests/test_progression_plan_filter.pytests/test_supervisor_planner.py
e3224bd to
117579d
Compare
Address Review: Round 1
|
Findings addressed or acknowledged. See Address Review comment.
|
@coderabbitai review |
|
xsovad06
left a comment
There was a problem hiding this comment.
PR Summary
This PR by @xsovad06 adds an LLM planning step (SupervisorPlanner) to the supervisor daemon, across 5 commits and 12 files (+1659/-10 lines). The planner assembles a resource snapshot (GitHub quota, CodeRabbit quota, CI budget, agent slots, open PRs, issue counts, recent failures) and the supervisor persona, calls the Anthropic API (Sonnet) to produce a structured PlanResult, then the deterministic TaskProgressionEngine filters its decisions against the approved plan. The plan can only subtract actions, never add or unblock. Gated behind supervisor.llm_planning = false (default off).
CI note: SonarCloud quality gate failed (likely pre-existing coverage threshold); all code-quality checks pass (lint, tests, static checks, invariants, integration tests).
Findings
No blocking or medium-severity findings. Two low-severity observations:
[LOW] Code Quality: Extra newline in PR table header
Location: sova/supervisor/planner.py:278
Problem: The table separator line has a trailing \n in the string literal. When joined with "\n".join(lines), this produces a double newline between the header separator and the first data row, breaking markdown table rendering. The LLM can still interpret the data, so this is cosmetic only.
Suggestion: Remove the trailing \n from the separator string:
"|---|-------|--------|----|-----------|\n", # current
"|---|-------|--------|----|-----------|", # fixed[LOW] Code Quality: Bare Bearer token values not caught by sanitizer
Location: sova/supervisor/planner.py:52-56
Problem: _SENSITIVE_RE catches key=value patterns like api_key=sk-abc123 and token: ghp_xyz but misses standalone Bearer eyJabc... values (without a preceding key: structure) and Authorization: Bearer xyz (where Authorization is the key, not bearer). Error messages from HTTP client libraries sometimes contain full authorization headers in this format.
Suggestion: Not urgent since _MAX_ERROR_LEN = 120 truncation is the primary defense and the regex catches the most common leak patterns. Consider adding Authorization to the pattern list if HTTP-layer errors become a concern.
Confirmed Bot Findings
Agree with CodeRabbit on sova/supervisor/planner.py:297: subprocess kill+reap on timeout. Confirmed as addressed in the current code (lines 262-266).
Agree with CodeRabbit on sova/supervisor/planner.py:348: raw error messages sanitized before LLM prompt. Confirmed as addressed via _sanitize_error() (lines 59-66, 346).
CodeRabbit's spawn_rebase finding (line 50) was correctly refuted by the author: spawn_rebase is already in _VALID_ACTIONS.
CodeRabbit's prompt injection finding (line 189) is valid in theory but mitigated in practice by the subtract-only invariant, require_approval=True default, and deterministic fallback. The worst case is a DoS (all actions become WAIT for one cycle), not unauthorized execution.
Verdict
Approve. The safety invariant is sound: the plan filtering in evaluate_all() (progression.py:247-271) correctly passes through non-actionable decisions unchanged, keeps only approved actionable decisions, and converts unapproved ones to WAIT. The plan never adds decisions or unblocks gated items. Error handling is thorough with graceful degradation to deterministic mode on any failure path. Config triple-registration is complete (model, loader via existing _NESTED_SECTIONS, settings_meta). The 42 tests provide strong coverage of parsing, filtering, error paths, and integration. The backward-compatible keyword-only parameter on evaluate_all() is clean.
What's Done Well
-
Safety-first architecture: the subtract-only invariant is elegant and verifiable. The plan sits above deterministic gates, never below them. This means even a compromised LLM response cannot cause unauthorized action execution.
-
Graceful degradation on every failure path: missing API key (log once, return None), HTTP timeout (return None), parse error (return None), generic exception (return None). The system silently falls back to the existing deterministic behavior with no user-visible disruption.
-
Clean integration with existing systems: reuses the
llm_suggestion_service.pyhttpx pattern for API calls, the_warned_no_keypattern for log suppression, and the persona loading infrastructure from #600. The daemon integration (_poll_progression) threads reasoning and deferred data through to the UI naturally.
(Posted as comment: GitHub does not allow self-reviews with formal approval/rejection state.)
117579d to
7bcfbef
Compare
Address Review: Round 2Addressing findings from the SOVA self-review (sova-review: approve).
All 5752 tests pass. All CI checks pass (SonarCloud quality gate is a pre-existing coverage threshold issue). |
… planning Introduces SupervisorPlanner class that assembles resource snapshots (GitHub quota, CodeRabbit quota, CI budget, agent slots, open PRs, issue counts, recent failures) and supervisor persona (#600), calls the Anthropic API (Sonnet via direct httpx) to produce a structured PlanResult with reasoning, approved actions, and deferred items. Persona braces are escaped before str.format() to prevent KeyError on user-authored curly braces in persona files. Uses --repo flag on gh pr list to avoid CWD dependency. Subprocess is killed and reaped on timeout to prevent orphaned gh processes. Error messages in the recent failures section are truncated and redacted (sensitive patterns like api_key, token, secret stripped) before inclusion in the LLM prompt. Gated behind supervisor.llm_planning config (default false, registered in settings_meta). Returns None on missing API key, timeout, or any error (falls back to deterministic mode silently). Closes #602
daemon._poll_progression() calls SupervisorPlanner.plan() before engine.evaluate_all(plan=plan) when llm_planning is enabled. evaluate_all() accepts optional plan: PlanResult parameter. When present, actionable decisions not in the plan's approved list are converted to WAIT with an informative reason. Non-actionable decisions (WAIT, BLOCKED, CHECKPOINT_NEEDED) pass through unchanged. The plan can only subtract actions, never add or unblock. Deterministic gates remain hard stops the LLM cannot override.
…isor UI supervisor_service stores reasoning and deferred alongside the pending plan. GET /api/supervisor/plan now returns reasoning (str|null) and deferred (list) fields. The pending actions panel shows the LLM's reasoning above action items (blue left border) and deferred items below (informational, no action buttons). Both sections are hidden when their data is absent for backward compatibility.
test_supervisor_planner.py (52 tests): dataclasses, API key handling, LLM call success/timeout/error, response parsing (valid, invalid action names, missing fields), persona brace escaping, context assembly, error message sanitization (redaction, truncation, authorization header, standalone bearer token), resource snapshot (happy path, all-fail), open PRs (success, CI states, timeout, empty, error), issue counts (by state, empty, error), recent failures (with data, empty, DB error), deferred/priority edge cases. test_progression_plan_filter.py (10 tests): plan filtering in evaluate_all() (None passthrough, approved kept, unapproved to WAIT, blocked preserved, action-type mismatch, empty plan). Supervisor service plan state management (reasoning + deferred storage). test_supervisor_daemon.py: add llm_planning=True integration test verifying planner is invoked and plan is passed to engine.
7bcfbef to
f8684cd
Compare
Address Review: Round 3Fixing SonarCloud coverage gap (64% -> 80%+ threshold on new code).
All 5771 tests pass. All CI checks pass including SonarCloud. |
…602) Spec covers dataclasses, context assembly, LLM call pattern, plan filtering, safety guarantees, config registration, dashboard UI changes, and testing strategy. Architecture.md updated with planner.py description: context assembly, Anthropic API pattern, plan filtering semantics, persona brace escaping, config gating.
f8684cd to
8e4bfef
Compare
|
Summary
supervisor.llm_planning = false(default): zero LLM calls, zero cost, zero behavior change until explicitly enabledChanges
Core planner (
sova/supervisor/planner.py):SupervisorPlannerclass with context assembly from 9 data sources, direct Anthropic API call via httpx (matchingllm_suggestion_service.pypattern), structured response parsing with validationPlanResult,PlannedAction,DeferredActionfrozen dataclassesstr.format()to preventKeyErroron user-authored{text}in persona files--repoflag ongh pr listto avoid CWD dependencyNoneon missing API key (logged once), timeout (30s), or any errorProgression engine (
sova/supervisor/progression.py):evaluate_all(plan=...)keyword-only parameter; when present, unapproved actionable decisions are converted to WAIT with informative reasonDaemon integration (
sova/supervisor/daemon.py):_poll_progression()calls planner before engine whenllm_planningis enabledset_pending_plan()for dashboard displayDashboard (
supervisor_service.py,supervisor.pyrouter,supervisor.html):set_pending_plan()accepts optionalreasoninganddeferredkwargsGET /api/supervisor/planreturnsreasoning(str|null) anddeferred(list) fieldsConfig (
models.py,settings_meta.py):supervisor.llm_planning: bool = False(no loader change needed, supervisor already in_NESTED_SECTIONS)Docs (
architecture.md, spec):docs/specs/issue-602-llm-planning.mdReview guidance
{curly braces}in their persona markdown--repoflag ongh pr listprevents the daemon from listing PRs for the wrong repo when CWD differs from project_dirTest plan
test_supervisor_planner.py(27 tests): dataclasses, API key handling, LLM call success/timeout/HTTP error/JSON parse error, response parsing (valid, invalid action names, invalid issue numbers, missing fields, empty actions), persona brace escaping, context assemblytest_progression_plan_filter.py(10 tests): plan filtering in evaluate_all() (None passthrough, approved kept, unapproved to WAIT, blocked preserved, action-type mismatch, empty plan filters all, pr_number preserved), supervisor service plan state managementCloses #602