diff --git a/.claude/agent-memory/cookbook.md b/.claude/agent-memory/cookbook.md index d40c18a9..53033c2e 100644 --- a/.claude/agent-memory/cookbook.md +++ b/.claude/agent-memory/cookbook.md @@ -68,6 +68,7 @@ Fully documented in `.claude/rules/` or `.claude/skills/`. One-line refs only. - **Fixed-width columns for right-aligned badge/button groups** -- wrap PR badge, state badge, and action button in a fixed-width container (e.g., `style="width:340px"`) with internal fixed-width divs. Without this, variable text widths cause ragged vertical alignment across rows. [confirmed: 1] - **Multi-action handoffs expand into a sub-row, not cramped inline** -- single-action handoffs (e.g., "Integrate PR") render on the main row. Multi-action handoffs (e.g., spec review with 4 buttons) expand into a second line below the title with summary text + buttons. [confirmed: 1] - **Deduplicate overlapping polls via `window._sova*` shared caches** -- when multiple JS modules poll the same endpoint (sidebar + page-specific), have the primary poller store its response in `window._sovaActiveAgents = {data, ts}` (or `window._sovaSystemMetrics`). Secondary consumers read from the cache with a staleness check (`Date.now() - cached.ts > 5000`). Server-side: add a `_metrics_cache` tuple with `time.monotonic()` + threading lock + 3s TTL on blocking functions like `get_system_metrics()`. PR #595. [confirmed: 1] +- **Save-poll race guard must cover ALL mutation paths, not just the primary save** -- when adding `_queueSaveInFlight` + `_queueLastSaved` grace period to prevent polling from overwriting just-saved state, the guard was initially only applied to `saveQueue()`. The review caught that `addToQueue()`, `removeFromQueue()`, and `clearQueue()` use separate API endpoints but write to the same underlying state, so they need the same `_queueLastSaved = Date.now()` timestamp. General rule: when guarding a race condition between polling and mutation, enumerate ALL functions that mutate the shared state. File: `sova/dashboard/templates/supervisor.html`. PR #656. [confirmed: 1] - **Clickable card pattern: cursor-pointer + stopPropagation on nested buttons** -- add `cursor-pointer hover:opacity-80` on clickable element with `onclick`. All nested interactive elements must call `event.stopPropagation()`. File: `sova/dashboard/templates/agents.html`. [confirmed: 1] - **Extract shared CAS patterns to avoid SonarCloud duplication gate** -- when multiple functions share validation + atomic CAS update logic (e.g., `resume_from_approval` and `reject_spec`), extract a shared helper parameterized by target status. Without this, the duplicated block triggers SonarCloud's 3% new code duplication threshold. PR #334. [confirmed: 1] - **DB-level tests needed for functions mocked in router tests** -- router tests that mock service functions (e.g., `reject_spec`, `_find_awaiting_approval_run`) provide integration coverage for the router but leave the service function at 0% coverage. Add separate DB-level tests that seed TaskRun records and call the service directly. PR #334: coverage went from 35.8% to 95.6%. [confirmed: 1] @@ -160,6 +161,8 @@ Fully documented in `.claude/rules/` or `.claude/skills/`. One-line refs only. - **Log diagnostic context before silent break/continue exits** -- when a `break` exits a loop on failure, log `stdout[:200]` and `stderr[:200]` before breaking. The final error message should include the reason, not just "could not be completed". PR #425 review. [confirmed: 1] - **New step `validate_output` must implement all four gate checks from architecture.md** -- new steps frequently implement two of the four required gate checks (unstaged diff, staged diff) but skip the other two (commits ahead of base via `git log {base}..HEAD --oneline`, untracked files via `git status --porcelain` lines starting with `??`). The architecture.md rule exists but gets missed when writing new steps. Checklist before shipping a new step: (1) `git diff --stat HEAD`, (2) `git diff --cached --stat`, (3) `git log {base}..HEAD --oneline`, (4) `git status --porcelain | grep "^??"`. PR #498 SOVA review finding. [confirmed: 1] - **`git stash pop` from a worktree subdir runs in the calling shell's CWD** -- `cd .claude/worktrees/X && git stash pop` in a Bash tool call does not persist the CWD change. The pop applies to the main repo index, not the worktree's. Use `git -C /absolute/path/to/worktree stash pop` instead, or apply changes directly to worktree files via Python scripts with absolute paths. [confirmed: 1] +- **Benchmark `log.sh` must run from the worktree CWD, not the main repo** -- `log.sh` uses `git branch --show-current` to resolve the issue number from `feat/issue-NNN`. Running from the main repo (on `main` branch) logs to the wrong issue file or falls back to whatever branch main is on. When running `/review-full` or `/pr` commands for a worktree branch, `cd` to the worktree first or use `bash /path/to/log.sh` from the worktree CWD. PR #656 session: `review_start`/`review_complete` logged to issue-650 instead of issue-632; PR #660 session: spec/review events logged to issue-650 instead of issue-657. [confirmed: 2] +- **Stale worktree directory + git checkout = main repo corruption** -- when a worktree is cleaned up (pruned, removed by background task) but the physical directory persists, `git` commands from inside that directory search upward and operate on the main repo. Running `git checkout ` from a stale worktree path silently switches the main repo to the feature branch. This causes: (1) main repo is no longer on `main`, (2) new worktree creation for the same branch fails, (3) files from the feature branch appear in the main repo. Diagnosis: `git worktree list` shows only one entry (main checkout on the wrong branch). Fix: `git checkout main` from the main repo directory. Prevention: after `git worktree remove`, verify the directory is deleted; if running commands in a worktree subdir, check `git worktree list` first. PR #660. [confirmed: 1] ## GitHub API @@ -261,6 +264,7 @@ Fully documented in `.claude/rules/` or `.claude/skills/`. One-line refs only. - **Polling loop caches must distinguish API failure from empty result** -- when an inner fetch function returns `[]` on both success (no items) and failure (API error), caching `[]` as a successful sync hides failures for the entire TTL. Return `None` on failure, `[]` on genuine empty result. Only set `_sync_cache[key]` on non-None returns. PR #636. [confirmed: 1] - **Single-flight lock for API-backed caches prevents concurrent cache misses** -- two concurrent callers can both pass the TTL check before either completes the fetch. Use `_sync_locks.setdefault(key, asyncio.Lock())` with a recheck inside the lock. Preserves `force=True` behavior by skipping the lock recheck on force. PR #636. [confirmed: 1] - **Cache keys must include all dimensions that distinguish entries** -- keying a per-PR cache by `pr_number` alone collides across repos in multi-project mode. Use `(repo, pr_number)` tuples. Same applies to any cache shared by a multi-project supervisor. PR #636. [confirmed: 1] +- **`_get_project_agents(slug)` must resolve project_dir from registry for non-default slugs** -- in supervisor/daemon context, `get_project_dir()` (contextvars) returns None because there's no HTTP request. Without a registry lookup fallback, all non-default projects get SOVA's own `_default_project_dir`, causing TaskRuns, validate_output, and GitHub API calls to target the wrong project. Fix: after `get_project_dir()` returns None and `slug != _DEFAULT_SLUG`, call `get_project_path(slug)` from `sova.config.registry`. General rule: any service function that resolves project context via contextvars must have a non-HTTP fallback for daemon/background callers. File: `sova/dashboard/services/agent_pool.py`. PR #660. [confirmed: 1] - **Invalidate API-backed caches on all state-changing paths** -- graph cache must be invalidated on agent spawn (`execute_decision`) AND epic close (`_try_close_epic`), not just one. Audit all code paths that call state-transition APIs and verify each invalidates the relevant cache. PR #636. [confirmed: 1] ## Refactoring / Code Quality @@ -394,6 +398,7 @@ Fully documented in `.claude/rules/` or `.claude/skills/`. One-line refs only. - **Daemon subprocess `gh` calls must pass `--repo` explicitly** -- the daemon's CWD may not be the project directory (multi-project mode, worktree context). Always pass `--repo {config.github_repo}` to `gh pr list`, `gh api`, etc. in daemon/supervisor subprocess calls. File: `sova/supervisor/planner.py:_get_open_prs()`. PR #625. [confirmed: 1] - **Identity-keyed in-memory state for multi-account isolation** -- global singletons (rate limit trackers, quota caches) conflate state across GitHub identities in multi-project setups. Replace `_tracker: T | None` with `_trackers: dict[str, T]`; key by `github_user` from config/adapter. All callers (adapter, dashboard services, progression engine) must pass the identity. PR #590 CodeRabbit. [confirmed: 1] - **Gate checks that block API calls must run before any API call in the same function** -- `evaluate_all()` called `build_dependency_graph()` (which calls `list_tasks()`) before checking the rate limit gate, wasting an API call during cooldown. Move the gate check to the top of any function that makes API calls as a side effect. Same applies to `auto_close_epics()` when building a fallback graph. PR #590 CodeRabbit. [confirmed: 1] +- **Execution-time liveness re-check for fallback paths without built-in guards** -- `evaluate_all()` checks `_check_already_running()` during evaluation, but `execute_decisions()` runs later. When the execution method delegates to `rollback_issue_state()` (which has its own concurrent-run guard), the primary path is safe. But fallback paths that call `adapter.transition_state()` directly need their own re-check, otherwise a dashboard-started agent between evaluation and execution gets its issue label overwritten. PR #655 CodeRabbit + SOVA review. [confirmed: 1] ## Auto-Retry / Failure Classification diff --git a/sova/dashboard/templates/supervisor.html b/sova/dashboard/templates/supervisor.html index c173b4bd..7bc7164d 100644 --- a/sova/dashboard/templates/supervisor.html +++ b/sova/dashboard/templates/supervisor.html @@ -42,6 +42,8 @@

Dependency Graph Low
+ @@ -200,6 +202,17 @@

Activity Stream

let _selectedNodeId = null; let _fitTransform = null; let _savedTransform = null; // user's current pan/zoom, preserved across re-renders +let _lastGraphData = null; + +function _lsGet(key) { + try { return localStorage.getItem(key); } catch { return null; } +} + +function _lsSet(key, val) { + try { localStorage.setItem(key, val); } catch { /* noop */ } +} + +let _hideDone = _lsGet('sova-graph-hide-done') === '1'; function _computeGraphKey(data) { // agent_elapsed_seconds is intentionally excluded: it changes every poll while an agent runs @@ -251,6 +264,21 @@

Activity Stream

function zoomOut() { if (_svgSel && _zoomBehavior) _svgSel.transition().duration(300).call(_zoomBehavior.scaleBy, 0.74); } function zoomFit() { if (_svgSel && _zoomBehavior && _fitTransform) _svgSel.transition().duration(350).call(_zoomBehavior.transform, _fitTransform); } +function toggleHideDone() { + _hideDone = !_hideDone; + _lsSet('sova-graph-hide-done', _hideDone ? '1' : '0'); + _updateHideDoneBtn(); + if (_lastGraphData) { + if (_svgSel) _savedTransform = d3.zoomTransform(_svgSel.node()); + renderGraph(_lastGraphData); + } +} + +function _updateHideDoneBtn() { + const btn = document.getElementById('hide-done-btn'); + if (btn) btn.textContent = _hideDone ? 'Show Done' : 'Hide Done'; +} + // -- Toggle / enable / disable -- let _toggleBusy = false; @@ -497,6 +525,7 @@

Setup Required

if (_svgSel) _savedTransform = d3.zoomTransform(_svgSel.node()); errorEl.classList.add('hidden'); + _lastGraphData = data; renderGraph(data); } catch (e) { loading.classList.add('hidden'); @@ -702,22 +731,42 @@

Setup Required

} }); + // Determine completed groups and visibility + const TOMBSTONE_H = 28; + + function isGroupCompleted(m) { + const nodes = msMap.get(m); + return !nodes.length || nodes.every(n => n.state === 'done' || n.state === 'closed'); + } + + const visibleMs = _hideDone + ? sortedMs.filter(m => !isGroupCompleted(m)) + : [...sortedMs]; + + const collapsedGroups = new Set(); + visibleMs.forEach(m => { + if (isGroupCompleted(m) && _lsGet(`sova-graph-expanded:${m}`) !== '1') { + collapsedGroups.add(m); + } + }); + // Arrange groups in a 2-column grid (even-index groups = left column, odd = right) - const col0MaxW = Math.max(...sortedMs.filter((_, i) => i % 2 === 0).map(m => groupLayouts.get(m).boxW), NW + GP * 2); + const col0MaxW = Math.max(...visibleMs.filter((_, i) => i % 2 === 0).map(m => groupLayouts.get(m).boxW), NW + GP * 2); const col0X = MARGIN; const col1X = col0X + col0MaxW + GGX; const groupPos = new Map(); let curY0 = MARGIN, curY1 = MARGIN; - sortedMs.forEach((m, i) => { - const { boxH } = groupLayouts.get(m); - if (i % 2 === 0) { groupPos.set(m, { x: col0X, y: curY0 }); curY0 += boxH + GGY; } - else { groupPos.set(m, { x: col1X, y: curY1 }); curY1 += boxH + GGY; } + visibleMs.forEach((m, i) => { + const h = collapsedGroups.has(m) ? TOMBSTONE_H : groupLayouts.get(m).boxH; + if (i % 2 === 0) { groupPos.set(m, { x: col0X, y: curY0 }); curY0 += h + GGY; } + else { groupPos.set(m, { x: col1X, y: curY1 }); curY1 += h + GGY; } }); - // Build global nodePos from group origin + local position + // Build global nodePos from group origin + local position (skip collapsed groups) const nodePos = {}; - sortedMs.forEach(m => { + visibleMs.forEach(m => { + if (collapsedGroups.has(m)) return; const { x: gx, y: gy } = groupPos.get(m); groupLayouts.get(m).nodeLPos.forEach((lp, id) => { const nx = gx + lp.lx, ny = gy + lp.ly; @@ -727,6 +776,11 @@

Setup Required

const allX = Object.values(nodePos).map(p => p.x + NW); const allY = Object.values(nodePos).map(p => p.y + NH); + collapsedGroups.forEach(m => { + const { x, y } = groupPos.get(m); + allX.push(x + groupLayouts.get(m).boxW); + allY.push(y + TOMBSTONE_H); + }); const contentW = (allX.length ? Math.max(...allX) + GP : 600) + MARGIN; const contentH = (allY.length ? Math.max(...allY) + GP : 400) + MARGIN; @@ -824,11 +878,50 @@

Setup Required

} // --- draw group boxes (behind edges and nodes) --- - sortedMs.forEach(m => { + visibleMs.forEach(m => { const { x, y } = groupPos.get(m); const { boxW, boxH, nodeLPos, subgroupMeta } = groupLayouts.get(m); const { bg, border, head } = groupPal.get(m); const memberIds = JSON.stringify([...nodeLPos.keys()]); + const completed = isGroupCompleted(m); + + if (collapsedGroups.has(m)) { + const grp = mainG.append('g') + .attr('class', 'group-box group-tombstone') + .attr('data-members', memberIds) + .style('cursor', 'pointer'); + + grp.append('rect') + .attr('x', x).attr('y', y).attr('width', boxW).attr('height', TOMBSTONE_H) + .attr('rx', 6) + .attr('fill', bg).attr('fill-opacity', 0.6) + .attr('stroke', border).attr('stroke-width', 1).attr('stroke-opacity', 0.4); + + const label = m.replace(/^Phase \d+:\s*/i, ''); + const tDisplay = label.length > 40 ? label.slice(0, 39) + '…' : label; + grp.append('text') + .attr('x', x + 10).attr('y', y + 18) + .attr('fill', head).attr('font-size', '9px').attr('font-weight', '600') + .attr('font-family', 'system-ui, sans-serif').attr('letter-spacing', '0.04em') + .attr('opacity', 0.7) + .text(tDisplay.toUpperCase()); + + const nodeCount = msMap.get(m).length; + grp.append('text') + .attr('x', x + boxW - 10).attr('y', y + 18) + .attr('text-anchor', 'end') + .attr('fill', '#a6e3a1').attr('font-size', '9px').attr('font-weight', '500') + .attr('font-family', 'system-ui, sans-serif') + .attr('opacity', 0.6) + .text(`${nodeCount} done`); + + grp.on('click', () => { + _lsSet(`sova-graph-expanded:${m}`, '1'); + if (_svgSel) _savedTransform = d3.zoomTransform(_svgSel.node()); + renderGraph(_lastGraphData); + }); + return; + } const grp = mainG.append('g').attr('class', 'group-box').attr('data-members', memberIds); @@ -854,6 +947,23 @@

Setup Required

.attr('font-family', 'system-ui, sans-serif').attr('letter-spacing', '0.04em') .text(display.toUpperCase()); + if (completed) { + grp.append('text') + .attr('x', x + boxW - GP).attr('y', y + GH - 8) + .attr('text-anchor', 'end') + .attr('fill', head).attr('font-size', '8.5px').attr('font-weight', '500') + .attr('font-family', 'system-ui, sans-serif') + .attr('opacity', 0.5) + .style('cursor', 'pointer') + .text('collapse') + .on('click', (event) => { + event.stopPropagation(); + _lsSet(`sova-graph-expanded:${m}`, '0'); + if (_svgSel) _savedTransform = d3.zoomTransform(_svgSel.node()); + renderGraph(_lastGraphData); + }); + } + // Subgroup dividers and labels (for large groups only) subgroupMeta.forEach(sg => { // Divider line above this subgroup (except first) @@ -1620,6 +1730,7 @@

Setup Required

} // -- Init -- +_updateHideDoneBtn(); loadStatus(); loadQuota(); loadCIBudget(); loadGraph(); loadDecisions(); loadPlan(); loadQueue(); loadSupervisorPersona(); // Poll every 30s as fallback; loadGraph re-renders only when data changes diff --git a/sova/supervisor/watchdog.py b/sova/supervisor/watchdog.py index c6f7d7d6..4ecabfc9 100644 --- a/sova/supervisor/watchdog.py +++ b/sova/supervisor/watchdog.py @@ -5,8 +5,8 @@ (pipeline not adopted, no output, step timeout, zombie process), and takes corrective action (warn via feed event, kill via stop_agent). -Killed agents flow through the existing _wait_and_finalize -> _schedule_retry() -path for auto-retry. The watchdog never independently retries. +Killed agents flow through the existing _wait_and_finalize path. +The watchdog never independently retries. """ from __future__ import annotations diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 7b7c052a..94a43ca6 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -421,12 +421,15 @@ def test_server_start_help(self) -> None: assert "host" in result.output.lower() or "port" in result.output.lower() def test_server_status_shows_not_running(self) -> None: + from unittest.mock import patch + from typer.testing import CliRunner from sova.cli.app import app runner = CliRunner() - result = runner.invoke(app, ["server", "status"]) + with patch("sova.scheduler.server.read_pid_file", return_value=None): + result = runner.invoke(app, ["server", "status"]) assert result.exit_code == 0 assert "not running" in result.output.lower() or "stopped" in result.output.lower()