feat(#483): manual task queue with priority ordering on supervisor page - #545
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:
WalkthroughAdds a persisted supervisor task queue with CRUD APIs and dashboard controls, applies queue order during task evaluation, and standardizes actionable-decision filtering. Tests cover queue persistence, deduplication, HTTP behavior, ordering, unknown issues, and empty-queue fallback. ChangesSupervisor task queue
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 5
🤖 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/config/models.py`:
- Line 529: Update the SupervisorConfig task_queue field validation so duplicate
IDs from TOML or environment-loaded configuration are removed while preserving
first-occurrence order, matching the API’s deduplication behavior. Add a direct
configuration regression test that constructs SupervisorConfig with duplicate
task_queue entries and verifies evaluate_all() processes each issue only once.
In `@sova/dashboard/routers/supervisor.py`:
- Around line 270-285: The queue persistence flow in _save_task_queue must
serialize concurrent read-modify-write operations under a shared lock and
replace sova.toml atomically; add coverage for concurrent updates. In
sova/dashboard/templates/supervisor.html lines 1313-1341, await the serialized
mutation, block overlapping reorder saves, and restore or reload the UI state
when persistence fails.
- Around line 25-30: Constrain issue numbers to positive integers across
QueueSetRequest.issue_numbers and QueueAddRequest.issue_number, and apply the
same validation to the issue_number path parameter used by the queue POST, PUT,
and skip_plan_item DELETE endpoints. Ensure non-positive values are rejected
before persistence or task-queue processing.
In `@sova/dashboard/templates/supervisor.html`:
- Around line 103-106: Add an accessible label for the issue-number input
`#queue-add-input` in the `#queue-add-form`, using an associated visible or
screen-reader-only label or an `aria-label`; keep the existing placeholder and
form behavior unchanged.
- Around line 1281-1291: Update the queue item rendering in the _queueData.map
flow to add keyboard-operable move-up and move-down controls with clear
accessible names, invoking the existing queue-reordering behavior. Keep
drag-and-drop intact, and disable or omit each control when moving in that
direction is not possible.
🪄 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: f4d410ba-b0a2-4a6c-a93b-d9031cd88973
⛔ Files ignored due to path filters (1)
.claude/agent-memory/cookbook.mdis excluded by!.claude/**and included by none
📒 Files selected for processing (7)
sova/config/models.pysova/dashboard/routers/supervisor.pysova/dashboard/settings_meta.pysova/dashboard/templates/supervisor.htmlsova/supervisor/progression.pytests/test_progression.pytests/test_supervisor_api.py
xsovad06
left a comment
There was a problem hiding this comment.
Review: BLOCK
This PR introduces a critical race condition in queue persistence where concurrent modifications can silently corrupt the task queue. The file I/O has no error handling, which will cause 500 errors on common scenarios like permission issues or disk full. The frontend optimistically updates on drag operations but doesn't revert on save failure, leaving the UI in an inconsistent state until the next poll.
8 findings (all to be addressed)
- [CRITICAL] [bug]
sova/dashboard/routers/supervisor.py:270: Classic read-modify-write race condition in queue persistence. Multiple concurrent requests (e.g., drag-reorder + API add) read the same TOML state, modify in-memory, and both write back. Last write wins, silently discarding the other's changes. This is especially problematic since the UI supports drag-and-drop which can trigger rapid concurrent saves. Fix: Wrap all queue mutations in a shared asyncio.Lock. Pattern:_queue_lock = asyncio.Lock(); async with _queue_lock: cfg = load_config(); queue = modify(queue); _save_task_queue(). Apply to add_to_queue, remove_from_queue, set_queue. The CodeRabbit finding is correct but understates the impact—this can lose user data. - [CRITICAL] [error-handling]
sova/dashboard/routers/supervisor.py:276: _save_task_queue has zero error handling for file I/O. toml_path.read_text() can raise PermissionError, FileNotFoundError (if deleted between exists check and read due to TOCTOU). tomlkit.parse() raises TOMLKitError on malformed TOML. write_text() raises OSError on disk full or permissions. All bubble up as 500 to user with no context. Fix: Wrap in try/except and raise HTTPException(503, detail='Failed to persist queue: {e}'). Pattern matches other dashboard file operations. Also: toml_path.exists() check has TOCTOU race—just catch FileNotFoundError on read instead. - [CRITICAL] [bug]
sova/supervisor/progression.py:212: Code references NON_ACTIONABLE_ACTIONS constant but the diff does not show its definition. If this constant is not defined at module level elsewhere in the file, this is a NameError that will crash all supervisor evaluations. The diff shows refactoring from local variables to a shared constant, but the constant definition is missing. Fix: Define at module level (after imports, before class):NON_ACTIONABLE_ACTIONS = frozenset({ProgressionAction.WAIT, ProgressionAction.BLOCKED, ProgressionAction.CHECKPOINT_NEEDED}). Use frozenset for immutability and O(1) membership checks. If it already exists outside diff context, disregard—but verify with grep. - [HIGH] [bug]
sova/dashboard/routers/supervisor.py:285: Non-atomic write via write_text() means process crash/kill during write leaves sova.toml partially written (corrupted). This breaks config loading for the entire project until manually fixed. Other high-value files in the codebase use temp-file-and-rename for atomicity. Fix: Use atomic write pattern:tmp = toml_path.with_suffix('.toml.tmp'); tmp.write_text(tomlkit.dumps(doc)); tmp.replace(toml_path). Path.replace() is atomic on POSIX. See sova/knowledge/crud.py for existing pattern. - [HIGH] [bug]
sova/dashboard/templates/supervisor.html:1280: saveQueue() optimistically updates _queueData before the server confirms success, then calls renderQueue() regardless of outcome. On network/server error, the UI shows the new order but the server still has the old order. Next poll (30s later) reverts it with no explanation—confusing UX. Same bug in drag-drop (line 1293). Fix: Only update _queueData inside 'if (res.ok)' block. On error, show toast and keep current state:if (!res.ok) { showToast('Failed to save queue order', 'error'); return; }. Remove renderQueue() from the outer scope—it's now inside the success branch. - [HIGH] [error-handling]
sova/dashboard/routers/supervisor.py:312: add_to_queue has no input validation—body.issue_number can be negative, zero, or absurdly large (INT_MAX). Negative/zero issue numbers are nonsensical in GitHub's numbering scheme and would cause confusing errors downstream when the supervisor tries to fetch them. Same issue in QueueSetRequest (line 307) and remove path. Fix: Add Pydantic validator:@field_validator('issue_number') @classmethod def _positive(cls, v: int) -> int: if v < 1: raise ValueError('issue_number must be positive'); return v. Apply to both request models. Alternatively, use Annotated[int, Field(gt=0)] on the field. - [HIGH] [error-handling]
sova/dashboard/templates/supervisor.html:1320: addToQueue handles 409 (duplicate) but silently ignores all other HTTP errors (400, 500, 503). User types a number, clicks Add, sees console.error but no UI feedback. They'll retry thinking it didn't register, potentially hitting the same error repeatedly. Fix: Add else branch after 409 check:else if (!res.ok) { showToast(Failed to add issue #${num}: ${res.status}, 'error'); }. Also check for catch block—network errors also need user-facing feedback. - [MEDIUM] [testing]
tests/test_supervisor_api.py:641: TestTaskQueueRouter has zero coverage for error paths: malformed TOML, read-only file, write failure, negative issue numbers, empty request body, concurrent modifications. These are all realistic failure modes that would return 500 or corrupt state, but they're untested. The spec explicitly lists 'edge cases' that should be covered. Fix: Add tests: test_malformed_toml_returns_503 (corrupt the TOML file, expect graceful error), test_negative_issue_number_rejected (once validation is added), test_readonly_toml_returns_503 (chmod 444, expect write failure). For concurrency, mock load_config to sleep and verify overlapping requests don't corrupt state.
Findings addressed in latest push.
Add task_queue field to SupervisorConfig with positive-int validation and settings metadata for dashboard UI. In the progression engine, when a task queue is configured, evaluate only queued issues in their specified order (skipping items not in the dependency graph). Extract NON_ACTIONABLE_ACTIONS to a module-level frozenset to eliminate duplicated set literals. Also fix auto_research default to False per architecture.md. Closes #483
Add queue management endpoints to the supervisor router: - GET /queue: read current queue with issue metadata - POST /queue: set entire queue (with deduplication) - POST /queue/add: add single issue (with gt=0 validation) - POST /queue/remove: remove issue from queue - POST /queue/clear: clear entire queue Include asyncio lock for concurrent queue mutations, atomic writes (temp file + rename) for sova.toml persistence, positive-int field_validator on QueueSetRequest, and proper HTTP error responses for I/O failures.
Add interactive queue management panel with: - Drag-and-drop reordering via HTML5 Drag and Drop API - Keyboard up/down buttons for accessible reordering - Add/remove issue controls with input validation - Optimistic UI updates with error rollback via showToast - Immutable array operations for drag/move (copy before splice)
Add tests for: - QueueSetRequest positive-int validation (rejects negative, zero) - QueueAddRequest gt=0 validation (rejects zero via 422) - Queue error paths: malformed TOML returns 503, write failures - Task queue ordering in progression engine (evaluate_all) - Auto_research default value verification
|



Summary
sova.tomlunder[supervisor] task_queueand exposed via CRUD API endpoints on the supervisor routerTaskProgressionEngine.evaluate_all()restricts evaluation to queued issues in queue order; empty queue preserves default evaluate-all behaviorChanges
Config (
sova/config/models.py,sova/dashboard/settings_meta.py)task_queue: list[int]field toSupervisorConfigwith empty list defaultSettingMetaentry for dashboard settings UI visibilityProgression engine (
sova/supervisor/progression.py)evaluate_all()to only evaluate queued issue numbers (preserving queue order) whentask_queueis non-emptyAPI (
sova/dashboard/routers/supervisor.py)GET /supervisor/queue,PUT /supervisor/queue(replace/reorder),POST /supervisor/queue(add single),DELETE /supervisor/queue/{issue_number}(remove single),DELETE /supervisor/queue(clear all)_save_task_queue()uses tomlkit for round-trip TOML preservation_deduplicate()helper preserves first-occurrence order on PUTDashboard UI (
sova/dashboard/templates/supervisor.html)Agent memory (
.claude/agent-memory/cookbook.md)Review guidance
sova.toml(file-based, single-user assumption). No locking on read-modify-write: acceptable for SOVA's single-user model but worth noting.task_queuefromself._config(which the daemon recreates each poll cycle fromload_config()), so TOML edits take effect on the next poll without restart._save_task_queuewrites directly tosova.tomlrather than going throughsettings_service.update_config()because the generic settings service handles scalar key-value pairs, not list values.Test plan
tests/test_progression.py::TestEvaluateAllTaskQueue: 4 tests covering queue filtering, order preservation, unknown issue skipping, and empty queue fallbacktests/test_supervisor_api.py::TestTaskQueueRouter: 8 integration tests covering all CRUD operations (get, add, add-duplicate, set/reorder, dedup, remove, remove-nonexistent, clear) plus TOML persistence verificationtests/test_supervisor_api.py::TestDeduplicate: 3 unit tests for the deduplication helperCloses #483