diff --git a/.gitignore b/.gitignore index a0380d05..bb0a1c9d 100644 --- a/.gitignore +++ b/.gitignore @@ -37,6 +37,8 @@ __pycache__/ venv/ ENV/ env/ +# Per-workspace uv managed CPython (UV_PYTHON_INSTALL_DIR); lives in clone + worktrees +.uv-python/ # uv .uv/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 3169d9c0..34749604 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,17 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ### Fixed +- **Per-task git worktrees — a coordinator PM's multiple in-progress roots no longer clobber each other on one shared checkout (F123).** A coordinator PM (Main / Cell) legitimately holds several in-progress roots at once, but its clone is a single checkout — so every fresh claim ran `git reset --hard` + `checkout -b` to the *new* branch and destroyed uncommitted tracked changes on the still-active *first* root (a live run showed `main-pm` ping-ponging two roots on one clone for ~13h). The reset's own comment assumed it was discarding "abandoned cruft from a finished task," but neither root was finished, and the git mutation was non-transactional with the DB claim (rollback restored DB fields, not the working tree). Each task now gets its own working tree via `git worktree add` under `{clone_root}/.worktrees/{task-short}/` on the same underlying clone, so a PM's roots each have an independent checkout and the F123 `reset --hard` dissolves entirely (a fresh worktree is clean by construction). The shared clone keeps the real `.git` object store, the per-project `.venv`, and `.uv-python`; each worktree gets a `.venv → ../../.venv` symlink so `uv` resolves the shared clone-root venv (no per-worktree re-sync), and `.uv-python` is now gitignored so every worktree inherits it. Branch-by-name git ops (`push`, `pull`, `fetch`, `pr_merge`, `diff`) run from the clone root as before; checkout/HEAD-moving ops (`create_branch`/`commit`/`rebase`/`checkout`) target the worktree. Spawn resolves the worktree from `current_task_id` on every spawn (never cached) and `-w`'s the container there; a resume/respawn re-attaches a pruned worktree before launch; claim-rollback `worktree remove --force`s on a mid-claim failure so a retry doesn't collide with a stale worktree; terminal cancel removes the worktree (the stale-claim reaper does not — it routes to `pending` for a re-claim that reuses it). The destructive `reset --hard origin/` in rebase recovery is pre-existing semantics, preserved. Invariants untouched: only the CEO merges master (no merge/release path touched), `/app/.venv` (the image-baked MCP-gateway venv) stays sacred, and the coordinator-PM concurrency exemption is unchanged — only the workspace resolution underneath became per-task. A real-`git`+`uv` integration test proves the clone root stays on `main` while two task worktrees each hold their own branch, and that `uv run` from a worktree resolves the clone-root venv through the symlink. + +- **The worktree switch's two missed cwd-dependent git ops now route to the worktree (F123 followup, both deploy-blockers).** The worktree switch wired `create_branch` + `commit` to the worktree but left two checkout-dependent ops resolving the clone root, both of which would have broken live. (1) `rebase_onto_base` does `git checkout ` + `git reset --hard origin/` in the resolved workspace — but post-worktree the branch is checked out in the linked worktree, so a `checkout` in the clone root is refused ("already checked out at ''"), wedging the `sync_branch` behind-base recovery and the PM's `rebase_pr_for_task` wedged-PR recovery with a fatal `GitCommandError`. `sync_task_branch` and `rebase_pr_for_task` now resolve the worktree via `_worktree_for_task` and rebase there (the `checkout` becomes a no-op on the already-checked-out branch). (2) `conventions_check_for_task` ran the validator with `--root `, and the validator reads `(root/rel).read_bytes()` — so it analyzed default-branch content, not the dev's worktree changes: newly-added files were absent from the clone root (false pass, the conventions block gate silently disabled) and modified files were validated at stale content. It now resolves the worktree and runs the validator there, so `i_am_done` / `pr_pass` gate against the real diff. + +- **Completed/merged tasks now clean up their per-task worktree (F123 followup).** Only `cancel()` and the `create_branch` rollback removed per-task worktrees, so every completed/merged task leaked its `{clone_root}/.worktrees/{task-short}/` on disk until the whole agent or project was deleted — accumulating clutter live (a PM doing many roots left N stale working trees). The two terminal→completed paths now remove the assignee's worktree best-effort: cell-PM `complete` (after the leaf PR merges) and CEO `ceo_approve` (after root→master merges). Removal is terminal-only — a dev task bounces `needs_revision` off the earlier review states and needs its worktree back, so cleanup fires only at `completed` (post-merge, branch truly done), never at `awaiting_qa`/`awaiting_documentation`/`awaiting_pm_review`/PR-merge. No-op for branchless/umbrella tasks (no worktree was ever cut). Best-effort (`check=False`, wrapped in try/except), so a removal failure never blocks completion. The stale-claim reaper's "don't remove, reuse on re-claim" rule is unchanged — only the terminal path is new. No merge/release path touched. +- **The give_me_work → claim path now enforces the per-dev lane barrier.** The lane order check (`has_earlier_incomplete_code_sibling`: a code leaf may not start while an earlier same-assignee sibling is still open) lived only on the orchestrator's spawn path and `i_am_idle`, so a developer who asked for work through `give_me_work` — or claimed a task directly via `i_will_work_on` — bypassed it and could start a later code leaf before the earlier one's PR merged, cutting a branch from a base that predates the sibling's unmerged changes. `give_me_work`'s pre-assigned path now filters through `_pending_not_lane_held` (a lane-held leaf is dropped, not offered), and `_run_claim_guards` refuses a direct claim of a lane-held code task (`invalid_state`, parked back to `pending` via `release_dependency_blocked_claim`). The predicate is CODE-only so coordinator PMs are naturally inert; the claim guard is fail-closed on a lookup error so a DB hiccup never lets an out-of-order start through. `is not True` keeps both paths inert under partial test mocks. + +- **Dev-task sequencing now chains undeclared-surface siblings on the same assignee.** The collision DAG only wired edges for dev tasks that declared a surface (`intends_to_touch` / `adds_migration` / `touches_shared`); a PM that delegated two dev tasks to the same developer without declaring surfaces wired no edge, so the later task could start while the earlier one's PR was still unmerged — the out-of-order start that wedged the merge. `wire_sibling_collision_dag` now falls back (only when no declared-surface collision edges exist) to chaining each same-`(project, assignee)` lane by `(priority, sequence)`: same-assignee siblings share a working tree, so the later one waits for the earlier. The lane is same-assignee scoped so cross-dev parallel work is untouched, and the edge lives in `dependency_ids` so it survives reassignment. Idempotent + incremental by construction (stable sort, `add_dependency` dedupes). + +- **Loop-prone notifications now have a bounded re-fire guard.** `TASK_ASSIGNMENT` / `REVIEW_REQUEST` / `DOCUMENTATION_REQUEST` / `BROADCAST` can be re-fired by a coordinator PM every tick while a task sits in a state, flooding inboxes. The existing DB purpose-dedup never fires for these four (`ACK_REQUIRED_BY_TYPE` marks them `requires_ack=False`, so the dedup is gated off), and the delivery path (`_persist_and_deliver`) had no dedup at all — so a wedged task re-sent the same signal every cycle, inflating each recipient's unacked set and driving respawn churn. A 60s Redis `SET NX` window per `(type, sender, recipient, task)` now coalesces the re-fire on both creation chokepoints (`NotificationService._create_notification` and `NotificationDeliveryService._persist_and_deliver`): the first fire acquires (marks) keys for fresh recipients, subsequent fires within the window are suppressed when no recipient was fresh, and the storm converges. Fail-open: Redis unavailable → never suppress (a notification is never dropped over dedup infra). One-shot types (`KNOWLEDGE_SHARE` / `MENTION` / `A2A_REQUEST`) bypass entirely (distinct content per send, no dedup key). + - **A whole-codebase logic-gap audit — roughly 140 concurrency, scoping, signal, and lifecycle gaps fixed.** The dominant body of this release. The categories: **cross-repo PR scoping** — `pr_number` and `branch_name` are per-repo but were stored and looked up unscoped, so two tasks on different repos sharing a PR number could merge the wrong repo's PR or skip the org's own in-flight integration PR; every PR-merge and branch-ownership lookup is now `project_id`-scoped, and `close_pull_request` / `pr_target` make `project_id` mandatory. **Advisory locks closing TOCTOU races** — per-agent on claim, per-parent on `delegate`, per-task on `open_pr` (preventing a milestone double-emit), plus an atomic server-side Redis probe-failure counter and a single-transaction `replace_chunks` (delete+insert) closing a reindex race. **Audit-row transactionality** — status-transition audit rows and the rework counter are written in-session in the caller's transaction (the old fire-and-forget path is gone), so the audit trail can't diverge from the state change. **Signal gaps** — `pr_fail` now pushes the reviewer's issues to the owning cell PM (the re-submit loop where a PM respawned into `needs_revision` blind and re-submitted the same PR is closed), and `fail_qa` routes a `needs_revision` dev task back to the dev, never the pool. **Asyncio cleanup** — `OptimalService.close()` cancels its startup indexing task before the periodic task and the plugin clear, so it can't write against closed plugins. **Conventions standard** — the validator now times out and reaps on hang, and the gate fails closed on resolution errors (a broken standard can no longer silently disable the gate). **WebSocket** — fan-out is non-blocking with finally-disconnect, idle-timeout, and dead-socket reaping on send error. **Orchestrator runtime** — it drains its fire-and-forget background set on shutdown and stops in lifespan shutdown before closing the DB; the probe-resume loop actually revives parked agents; the grok auth token is refreshed before expiry and parked (not crash-retried) when missing. **Release executor** — every subprocess (git/make/gh/clone) is deadline-bounded and it fails closed on a git add/commit before push. Dozens more across org-memory (private-leak closures, playbook index/unindex as a post-commit step so the RAG corpus never leads the status transaction), the reaper, the provider-park/overload break, and the live-chat bridges. The full categorized tracker lives in `docs/internal` (gitignored). - **The 2026-06-27 live-run meltdown cluster — root-caused and closed.** A run hit several compounding wedges at once, each TDD-fixed and verified green: a `main_pm` assigned a `code`-typed task is a structural impossibility (a coordinator PM does no coding) and is now hard-rejected at the gate; `cell_pm_complete` resolved a merge by global `pr_number` and merged the wrong repo's PR (closed by the cross-repo `project_id` scoping above); `submit_root` re-submitted an unchanged PR into an infinite `pr_fail` loop (now hard-gated); `fail_qa` bounced a dev task to the pool instead of back to the dev; a `note(scope='handoff')` with an empty section crashed the note path and tripped a PM respawn loop; the MegaTask four-layer hierarchy (umbrella → root → cell → dev) hit a depth cap sized for three layers; and the durable respawn counter's persist raced under fire-and-forget (an atomic upsert closes it). diff --git a/roboco/models/runtime.py b/roboco/models/runtime.py index a3f6150c..2fcd87b6 100644 --- a/roboco/models/runtime.py +++ b/roboco/models/runtime.py @@ -30,6 +30,12 @@ class SpawnGitContext: project_slug: str | None = None branch_name: str | None = None + # Short id (task id[:8]) of the task whose per-task worktree the agent + # must edit in. Set only for tasks that carry a branch (a real worktree + # exists under {clone_root}/.worktrees/{task_short_id}/); branchless + # coordination roots leave it None so the spawn cwd falls back to the + # clone root. + task_short_id: str | None = None @dataclass diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 6bf19f53..7dd4f319 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -470,6 +470,42 @@ def _agent_workspace_path(project_slug: str, team: str, agent_id: str) -> str: return f"/data/workspaces/{project_slug}/{team}/{agent_id}" +def _agent_worktree_path( + project_slug: str, team: str, agent_id: str, task_short_id: str +) -> str: + """Per-task worktree path inside the container (F123). + + Each task with a branch gets its own working tree under the clone root at + ``{clone_root}/.worktrees/{task_short_id}/`` so a coordinator PM's parallel + roots (or a dev's parallel tasks) never clobber one shared checkout. + """ + return ( + f"/data/workspaces/{project_slug}/{team}/{agent_id}/.worktrees/{task_short_id}" + ) + + +def _agent_cwd_path( + project_slug: str, + team: str, + agent_id: str, + git_context: SpawnGitContext | None, +) -> str: + """The container cwd + Edit/Write scope for a workspace role (F123). + + A task carrying a branch edits in its per-task worktree; a branchless or + no-task spawn stays at the clone root. ONE formula shared by + ``_append_workspace_cwd`` (docker ``-w``) and ``_get_role_permissions`` + (Edit/Write allowlist via ``_prepare_agent_spawn``) so the cwd and the + allowlist scope can never drift to different paths. + """ + clone_root = _agent_workspace_path(project_slug, team, agent_id) + if git_context and git_context.task_short_id: + return _agent_worktree_path( + project_slug, team, agent_id, git_context.task_short_id + ) + return clone_root + + def _cell_workspace_path(project_slug: str, team: str) -> str: """Cell-level workspace path (documenter scope). @@ -1656,10 +1692,15 @@ class AgentOrchestrator: project_slug = task.get("project_slug") if not project_slug: return None - return SpawnGitContext( - project_slug=project_slug, - branch_name=task.get("branch_name"), - ) + branch_name = task.get("branch_name") + ctx = SpawnGitContext(project_slug=project_slug, branch_name=branch_name) + # A branch-bearing task edits in a per-task worktree keyed by the short + # id; a branchless coordination root (umbrella / no-project product + # root) has no worktree, so task_short_id stays None and the spawn cwd + # falls back to the clone root. + if branch_name and task.get("id"): + ctx.task_short_id = str(task["id"])[:8] + return ctx def _fire_audit( self, @@ -1758,10 +1799,12 @@ class AgentOrchestrator: branch_name, project_slug = row if not project_slug: return None - return SpawnGitContext( - project_slug=project_slug, - branch_name=branch_name, + ctx = SpawnGitContext( + project_slug=project_slug, branch_name=branch_name ) + if branch_name and task_id: + ctx.task_short_id = str(task_id)[:8] + return ctx except Exception as e: logger.warning( "Could not derive git context from task_id", @@ -1908,16 +1951,26 @@ class AgentOrchestrator: if not model: model = route.model_name - workspace_path = _agent_workspace_path(project_slug, team, agent_id) cell_workspace_path = _cell_workspace_path(project_slug, team) + # The agent's edit scope + container cwd: the per-task worktree when + # the task carries a branch (F123), else the clone root. Routed through + # _agent_cwd_path so the Edit/Write allowlist (_generate_agent_settings + # -> _get_role_permissions) and the docker -w (_append_workspace_cwd) + # resolve the SAME path. + cwd_path = _agent_cwd_path(project_slug, team, agent_id, git_context) + + # Re-attach the task's worktree before the container launches with -w + # pointing at it (F123). A pruned/evicted worktree would start the + # agent in a missing dir; idempotent re-add, no-op for branchless spawns. + await self._ensure_worktree_before_spawn( + git_context, project_slug, team, agent_id, task_id + ) agent_settings_path = self._generate_agent_settings( - agent_id, canonical_role, workspace_path, cell_workspace_path + agent_id, canonical_role, cwd_path, cell_workspace_path ) - briefing_path = await self._write_agent_briefing( - agent_id, task_id, workspace_path - ) + briefing_path = await self._write_agent_briefing(agent_id, task_id, cwd_path) await self._ensure_agent_image(agent_id) mcp_config_path = await self._generate_mcp_config(agent_id, git_context) @@ -1945,6 +1998,81 @@ class AgentOrchestrator: self._instances[agent_id] = instance return config, instance, agent_settings_path + async def _ensure_worktree_before_spawn( + self, + git_context: SpawnGitContext | None, + project_slug: str, + team: str, + agent_id: str, + task_id: str | None, + ) -> None: + """Re-attach the task's per-task worktree before the container starts. + + The container launches with ``-w`` at the worktree; a pruned/evicted + worktree (reaper, disk pressure, manual cleanup while the agent was + down) would start the agent in a missing directory. Idempotent — + ``ensure_worktree_for_resume`` is a no-op when the worktree is present + and re-adds it (no ``-b``) from the surviving branch ref when pruned. + No-op for branchless / no-task spawns (no worktree). + + A fatal git-state failure (``WorkspaceError`` — the branch ref is gone, + so the worktree can't be re-added) releases the claim and aborts the + spawn so the next claim rebuilds the worktree via ``create_branch`` + rather than launching the container at a missing ``-w`` path. A + transient failure (DB/other) aborts without releasing — the next tick + retries the same claim. + """ + if not (git_context and git_context.task_short_id and git_context.branch_name): + return + clone_root = Path(_agent_workspace_path(project_slug, team, agent_id)) + worktree = Path( + _agent_worktree_path( + project_slug, team, agent_id, git_context.task_short_id + ) + ) + from roboco.db.base import get_db_context + from roboco.services.workspace import WorkspaceError, WorkspaceService + + try: + async with get_db_context() as db: + await WorkspaceService(db).ensure_worktree_for_resume( + clone_root, worktree, git_context.branch_name + ) + except WorkspaceError as e: + # Fatal git state: the branch ref is gone, so the worktree cannot be + # re-added here. Release the claim so the next claim rebuilds the + # worktree via create_branch, and abort before docker run -w lands + # on a missing path. The release is best-effort (suppressed) so a + # release failure never masks the fatal error. + logger.error( + "worktree ensure failed (fatal); releasing claim for rebuild", + agent_id=agent_id, + task_short_id=git_context.task_short_id, + error=str(e), + ) + if task_id: + with contextlib.suppress(Exception): + await self._release_claim_to_pending(task_id) + raise AgentReadinessError( + f"worktree ensure failed for {agent_id}" + f" (task={task_id}, branch={git_context.branch_name}): {e};" + f" claim released for rebuild" + ) from e + except Exception as e: + # Transient (DB hiccup, etc.): abort so we don't launch at a + # possibly-missing path, but do NOT release — a fresh claim would + # not help and re-cloning is destructive. Next tick retries. + logger.warning( + "worktree ensure failed (transient); aborting spawn", + agent_id=agent_id, + task_short_id=git_context.task_short_id, + error=str(e), + ) + raise AgentReadinessError( + f"worktree ensure failed (transient) for {agent_id}" + f" (task={task_id}): {e}; will retry next tick" + ) from e + async def _launch_spawn( self, task_id: str | None, @@ -2357,7 +2485,15 @@ class AgentOrchestrator: team = get_agent_team(config.agent_id) or "" project = _resolve_project_slug_from_git_context(config.git_context) if role in AgentOrchestrator._ROLES_WITH_AGENT_WORKSPACE: - cmd.extend(["-w", _agent_workspace_path(project, team, config.agent_id)]) + # Per-task worktree when the task has a branch (F123), else the + # clone root. _agent_cwd_path is the SAME formula the Edit/Write + # allowlist is built from, so -w and the allowlist match exactly. + cmd.extend( + [ + "-w", + _agent_cwd_path(project, team, config.agent_id, config.git_context), + ] + ) elif role in AgentOrchestrator._ROLES_WITH_CELL_WORKSPACE: cmd.extend(["-w", _cell_workspace_path(project, team)]) @@ -5410,7 +5546,12 @@ class AgentOrchestrator: continue restored[(r.agent_slug, str(r.task_id))] = { "count": r.count, - "last_status": r.last_status, + # Re-stamp to the LIVE status (mirrors the last_check re-stamp + # above): a pre-restart last_status is as stale w.r.t. post-restart + # reality, and a status mismatch across the restart gap would + # otherwise disarm the breaker on the first post-restart spawn and + # re-burn the whole strike threshold against a still-wedged task. + "last_status": norm, "last_check": restore_now, "tracing_resets": r.tracing_resets, "notified": r.notified, diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 5480d4c2..b90efd59 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -773,7 +773,9 @@ class Choreographer: # list_assigned_for_agent (ordered by priority/updated_at — pending # could rank behind in_progress rows) and the PM path checked # awaiting_* queues but not the pre-assigned pending case. - pre_assigned = await self._deps.task.list_pending_for_agent(agent_id) + pre_assigned = await self._pending_not_lane_held( + await self._deps.task.list_pending_for_agent(agent_id) + ) if pre_assigned: t = pre_assigned[0] return Envelope.ok( @@ -970,7 +972,35 @@ class Choreographer: # unless the task is currently claimed/in_progress. await self.task.release_dependency_blocked_claim(task.id) return guard - return None + return await self._lane_claim_guard(task) + + async def _lane_claim_guard(self, task: Any) -> Envelope | None: + """Refuse a code leaf behind an earlier open same-assignee sibling. + + The out-of-order start wedge: a later PR cut from a base that predates + the earlier sibling's unmerged changes. CODE-only predicate -> + coordinator PMs are inert. Fail-closed on lookup error so a DB hiccup + never lets an out-of-order claim through. + """ + try: + lane_held = await self.task.has_earlier_incomplete_code_sibling(task) + except Exception: + return Envelope.invalid_state( + message=( + f"lane order check failed for task {task.id}; retry give_me_work." + ), + remediate="call give_me_work() to re-fetch available work", + ) + if lane_held is not True: + return None + await self.task.release_dependency_blocked_claim(task.id) + return Envelope.invalid_state( + message=( + f"task {task.id} waits behind an earlier open task in your " + "code lane; start that one first." + ), + remediate="call give_me_work() to pick up the earlier task", + ) async def _non_terminal_subtask_ids(self, parent_task_id: UUID) -> str: """Return a human-readable comma-separated list of non-terminal subtasks. diff --git a/roboco/services/git.py b/roboco/services/git.py index a192bee6..a0adc004 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -126,6 +126,33 @@ _GIT_EXECUTOR = ThreadPoolExecutor( _SLOW_GIT_OP_MS = 5000.0 +def resolve_git_dir(workspace: Path) -> Path | None: + """Resolve the real ``.git`` directory for a workspace or linked worktree. + + A normal clone's ``.git`` is a directory. A linked worktree's ``.git`` is a + *file* containing ``gitdir: `` pointing into the clone root's + ``.git/worktrees//``. Callers that rglob locks / parse config must go + through here, not assume ``workspace / ".git"`` is a directory. + + Returns the resolved git dir, or None if the workspace has no git metadata. + """ + dot_git = workspace / ".git" + if dot_git.is_dir(): + return dot_git + if dot_git.is_file(): + try: + first = dot_git.read_text().splitlines()[0].strip() + except (OSError, IndexError): + return None + if not first.startswith("gitdir: "): + return None + target = Path(first[len("gitdir: ") :].strip()) + if not target.is_absolute(): + target = (workspace / target).resolve() + return target if target.is_dir() else None + return None + + def _remove_stale_git_locks(workspace: Path) -> None: """Best-effort removal of orphaned ``.git/**/*.lock`` files. @@ -137,9 +164,13 @@ def _remove_stale_git_locks(workspace: Path) -> None: the timeout fires the git process is dead, so its orphaned locks are safe to remove. Best-effort: any error (no .git, race with a real process) is swallowed — this only ever *helps*, never blocks. + + Worktree-aware (F123): a linked worktree's ``.git`` is a gitdir pointer — + route through ``resolve_git_dir`` so locks inside ``.git/worktrees//`` + are reached. """ - git_dir = workspace / ".git" - if not git_dir.is_dir(): + git_dir = resolve_git_dir(workspace) + if git_dir is None or not git_dir.is_dir(): return try: for lock in git_dir.rglob("*.lock"): @@ -712,6 +743,31 @@ class GitService(BaseService): ) return task + @staticmethod + def _worktree_for_task(clone_root: Path, task_id: UUID) -> Path: + """Per-task worktree path under a clone root (F123). + + Matches ``create_branch``'s ``{clone_root}/.worktrees/{task_id[:8]}`` + layout so commit/checkout/rebase paths resolve the same worktree the + claim cut and the spawn cwd pointed the agent at. + """ + return clone_root / ".worktrees" / str(task_id)[:8] + + async def _ensure_worktree_for_commit( + self, clone_root: Path, worktree: Path, branch: str | None + ) -> None: + """Ensure a task's worktree is attached before a cwd-dependent git op. + + Resume re-adds a pruned worktree, but a worktree can also be evicted + mid-task (disk pressure, manual cleanup); a commit/checkout against a + missing dir fails opaquely. Idempotent — no-op when the worktree is + present, re-adds (no ``-b``) from the surviving branch ref when pruned. + """ + if not branch: + return + workspace_service = get_workspace_service(self.session) + await workspace_service.ensure_worktree_for_resume(clone_root, worktree, branch) + async def _assert_on_task_branch( self, workspace: Path, task_branch: str | None ) -> None: @@ -835,7 +891,14 @@ class GitService(BaseService): """ if data.task_id is not None: task = await self._assert_task_owned_with_branch(data.task_id, agent_id) - workspace = await self.get_workspace(data.project_slug, agent_id) + clone_root = await self.get_workspace(data.project_slug, agent_id) + # Commit inside the task's per-task worktree (F123), not the shared + # clone — the clone's HEAD may be parked on the default branch. + worktree = self._worktree_for_task(clone_root, data.task_id) + await self._ensure_worktree_for_commit( + clone_root, worktree, task.branch_name + ) + workspace = worktree await self._assert_on_task_branch(workspace, task.branch_name) else: workspace = await self.get_workspace(data.project_slug, agent_id) @@ -993,66 +1056,55 @@ class GitService(BaseService): timeout=_network_git_timeout(), ) - # The dev workspace is one persistent clone shared across this dev's - # tasks, so a finished/abandoned prior task can leave it dirty and on a - # sibling branch. Without a clean tree the base + feature checkouts below - # fail; and because this git work is a side-effect that runs AFTER the - # claim's DB transition has committed, a failed checkout leaves the - # workspace on the wrong branch while the task is already marked - # assigned — so the dev's next commit is rejected with BRANCH_MISMATCH. - # This runs only on a FRESH claim (resume short-circuits in _dev_reentry - # before reaching here), so any uncommitted changes are abandoned cruft - # from a finished task — safe to discard. `reset --hard` clears tracked - # changes; the gitignored .venv (and other ignored files) are untouched. - await self._run_git(workspace, ["reset", "--hard"], check=False) + # --- F123: per-task worktree, not a shared-clone checkout. --- + # The dev clone is one persistent checkout shared across this dev's + # tasks, and a coordinator PM may hold several in_progress roots at + # once. The old `reset --hard` + `checkout -b` on the shared clone + # clobbered a still-active sibling root's working tree (live on NAS: + # main-pm ping-ponged two roots on one clone). Each task now gets its + # own linked worktree under {clone_root}/.worktrees/{task-short}/ via + # `git worktree add`; the clone's HEAD is never moved by a claim, so + # sibling roots' trees are isolated. This runs only on a FRESH claim + # (resume short-circuits in _dev_reentry before reaching here). + worktree_path = workspace / ".worktrees" / str(task_id)[:8] - base_branch = await self._checkout_base_with_fallback( - workspace, base_branch, default_branch, task_id + # Branch from the fetched remote tip (matches the old + # `merge --ff-only origin/` intent — build on the latest remote + # base, not a stale local checkout). Fall back to origin/ if + # isn't on the remote yet (the ls-remote above already retargets + # base_branch to default in that case; this covers a residual miss). + base_ref = f"origin/{base_branch}" + ref_check = await self._run_git( + workspace, ["rev-parse", "--verify", "--quiet", base_ref], check=False + ) + if ref_check.returncode != 0: + base_ref = f"origin/{default_branch}" + base_branch = default_branch + + # ensure_worktree: `git worktree add -b ` for a new + # branch, or `worktree add ` (reuse) for an existing on-disk + # branch (a prior attempt that rolled back DB fields but left the + # branch). Idempotent on an already-present worktree (re-claim). + workspace_service = get_workspace_service(self.session) + await workspace_service.ensure_worktree( + workspace, worktree_path, branch_name, base_ref ) - # Fast-forward the checked-out base to the freshly-fetched remote tip. - # A plain `git pull origin ` is fragile in automation: if the - # local base has diverged at all it aborts with exit 128 ("Need to - # specify how to reconcile divergent branches" / refusing to merge - # unrelated histories), which then blows up the whole claim. We only - # ever want the latest remote base before cutting a branch, so a local - # `merge --ff-only origin/` is the right intent — and it uses the - # ref the scoped fetch above already updated (no second network call). - # check=False: a non-fast-forward (divergent local) or a base that - # isn't on the remote yet leaves the checked-out base as the branch - # point instead of aborting branch creation. - await self._run_git( + # An existing branch with no commits of its own — a dependency-blocked + # task re-claimed after its upstream merged — is re-pointed at the fresh + # base so the agent builds on the current tip. Runs on the WORKTREE, + # never the shared clone. A freshly `-b`'d branch is already at base, so + # this is a no-op for new branches; a branch carrying real work + # (unique > 0) is left exactly as-is. + unique = await self._run_git( workspace, - ["merge", "--ff-only", f"origin/{base_branch}"], + ["rev-list", "--count", f"{base_ref}..{branch_name}"], check=False, ) - # Idempotent branch creation: a prior attempt may have created the - # branch on disk but failed before the DB recorded branch_name (the - # claim rolls back its fields, but the on-disk branch persists). A - # plain `checkout -b` then fails "already exists" (exit 128), and the - # resulting error-handling cascade is how a retry spirals. Switch to - # the existing branch instead. - created = await self._run_git( - workspace, ["checkout", "-b", branch_name], check=False - ) - if created.returncode != 0: - await self._run_git(workspace, ["checkout", branch_name]) - # The branch already existed on disk. If it carries no commits of - # its own — a dependency-blocked task branched before its upstream - # merged into the integration branch, then released and re-claimed — - # re-point it at the freshly-pulled base so the agent builds on the - # current integration tip, not a stale snapshot. Guarded on "no - # commits unique to the branch": a branch with real work is left - # exactly as-is. - unique = await self._run_git( - workspace, - ["rev-list", "--count", f"{base_branch}..{branch_name}"], - check=False, + if unique.returncode == 0 and unique.stdout.strip() == "0": + await self._run_git( + worktree_path, ["reset", "--hard", base_ref], check=False ) - if unique.returncode == 0 and unique.stdout.strip() == "0": - await self._run_git( - workspace, ["reset", "--hard", base_branch], check=False - ) await self._run_git( workspace, ["push", "-u", "origin", branch_name], @@ -3773,14 +3825,18 @@ class GitService(BaseService): raise NotFoundError("Project for task", str(task.id)) workspace_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id) - workspace = await self.get_workspace(project.slug, agent_id=workspace_agent_id) + clone_root = await self.get_workspace(project.slug, agent_id=workspace_agent_id) git_token = await self._get_project_token_or_raise(project.slug) - owner, repo = self._parse_github_remote(workspace) + owner, repo = self._parse_github_remote(clone_root) refs = await self._get_pr_refs(owner, repo, pr_number, git_token) if refs is None: return {"status": "unknown"} head_branch, base_branch = refs + # Rebase inside the per-task worktree (F123): the PR head branch is + # checked out there, so a checkout in the clone root would be refused. + workspace = self._worktree_for_task(clone_root, require_uuid(task.id)) + await self._ensure_worktree_for_commit(clone_root, workspace, head_branch) return await self.rebase_onto_base( workspace, head_branch=head_branch, @@ -3816,8 +3872,13 @@ class GitService(BaseService): if project is None: raise NotFoundError("Project for task", str(task.id)) workspace_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id) - workspace = await self.get_workspace(project.slug, agent_id=workspace_agent_id) + clone_root = await self.get_workspace(project.slug, agent_id=workspace_agent_id) git_token = await self._get_project_token_or_raise(project.slug) + # Rebase inside the per-task worktree (F123): the branch is checked out + # there, so a checkout in the clone root would be refused ("already + # checked out at ''") and the behind-base recovery loop dies. + workspace = self._worktree_for_task(clone_root, require_uuid(task.id)) + await self._ensure_worktree_for_commit(clone_root, workspace, task.branch_name) return await self.rebase_onto_base( workspace, head_branch=task.branch_name, @@ -4248,9 +4309,13 @@ class GitService(BaseService): and downstream gateway code only consume `sha`; the rest is included so we don't have to invent a new shape later. """ - workspace = await self._workspace_for_branch( + clone_root = await self._workspace_for_branch( branch_name, actor_agent_id=actor_agent_id ) + # Commit inside the task's per-task worktree (F123), not the shared + # clone — keyed by the task id so it matches create_branch's layout. + workspace = self._worktree_for_task(clone_root, task_id) + await self._ensure_worktree_for_commit(clone_root, workspace, branch_name) await self._assert_on_task_branch(workspace, branch_name) # Stage files explicitly when provided; otherwise stage everything @@ -4318,7 +4383,7 @@ class GitService(BaseService): branch = task.branch_name if not branch: return {"findings": [], "could_not_run": False} - workspace = await self._workspace_for_branch( + clone_root = await self._workspace_for_branch( branch, actor_agent_id=actor_agent_id ) changed = await self.list_changed_files( @@ -4332,6 +4397,12 @@ class GitService(BaseService): } if not changed: return {"findings": [], "could_not_run": False} + # Validate the worktree's working tree (F123): the dev's changes live in + # the per-task worktree, not the clone root (which sits on the default + # branch). A validator run against the clone root reads stale/default + # content and false-passes on newly-added files. + workspace = self._worktree_for_task(clone_root, require_uuid(task.id)) + await self._ensure_worktree_for_commit(clone_root, workspace, branch) return await self._run_conventions_validator(workspace, changed) async def _run_conventions_validator( diff --git a/roboco/services/notification.py b/roboco/services/notification.py index 3af82709..a694e1ac 100644 --- a/roboco/services/notification.py +++ b/roboco/services/notification.py @@ -17,6 +17,7 @@ from roboco.db.tables import AgentTable, NotificationTable from roboco.foundation.policy.communications import ACK_REQUIRED_BY_TYPE from roboco.models import NotificationPriority, NotificationType from roboco.models.notification import CreateNotificationParams +from roboco.services.notification_dedup import all_recipients_recently_notified from roboco.utils.converters import require_uuid if TYPE_CHECKING: @@ -483,6 +484,25 @@ class NotificationService: subject=params.subject[:80], ) return + # Re-fire guard for loop-prone types: a 60s Redis SET-NX window + # coalesces the per-tick re-notify storm the DB dedup below skips + # (these types are requires_ack=False). Fail-open on Redis down. + if await all_recipients_recently_notified( + ntype=params.notification_type, + from_agent=from_agent_uuid, + recipients=to_agents_uuids, + related_task_id=params.related_task_id, + ): + logger.info( + "Suppressed re-fire notification (loop-prone, recent window)", + from_agent=str(from_agent_uuid), + type=params.notification_type.value, + related_task_id=str(params.related_task_id) + if params.related_task_id is not None + else None, + to_agents=[str(a) for a in to_agents_uuids], + ) + return # Purpose-based dedup (CEO directive, 2026-06-10): do NOT create a # second notification for the SAME purpose — same sender, same type, # same task, overlapping recipients — while a prior one is still diff --git a/roboco/services/notification_dedup.py b/roboco/services/notification_dedup.py new file mode 100644 index 00000000..c8a0692c --- /dev/null +++ b/roboco/services/notification_dedup.py @@ -0,0 +1,91 @@ +"""Bounded re-fire guard for loop-prone notification types. + +TASK_ASSIGNMENT / REVIEW_REQUEST / DOCUMENTATION_REQUEST / BROADCAST can be +re-fired by a PM every tick while a task sits in a state, flooding inboxes. +The existing DB dedup is gated to action-required types only and never fires +for these four, so a short Redis SET-NX window per (type, sender, recipient, +task) suppresses the re-fire here. Fail-open: Redis unavailable → never +suppress (a notification is never dropped because the dedup infra is down). +One-shot types (KNOWLEDGE_SHARE / MENTION / A2A_REQUEST) bypass entirely. +""" + +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +import redis.asyncio as redis + +from roboco.config import settings +from roboco.models import NotificationType + +if TYPE_CHECKING: + from collections.abc import Sequence + from uuid import UUID + +logger = logging.getLogger(__name__) + +# Loop-prone: a coordinator re-fires these every tick while the task sits in a +# state. One-shot types (knowledge share, mention, a2a request) are excluded. +_LOOP_PRONE_TYPES = frozenset( + { + NotificationType.TASK_ASSIGNMENT, + NotificationType.REVIEW_REQUEST, + NotificationType.DOCUMENTATION_REQUEST, + NotificationType.BROADCAST, + } +) + +# 60s: long enough to coalesce a re-fire storm, short enough that a genuine +# follow-up (state actually changed, a new ack window) still lands. +_DEDUP_TTL_SECONDS = 60 + + +def _key( + ntype: NotificationType, + from_agent: UUID | str, + recipient: UUID | str, + related_task_id: UUID | str | None, +) -> str: + task_part = str(related_task_id) if related_task_id is not None else "none" + return f"roboco:notif_dedup:{ntype.value}:{from_agent}:{recipient}:{task_part}" + + +async def all_recipients_recently_notified( + *, + ntype: NotificationType, + from_agent: UUID | str | None, + recipients: Sequence[UUID | str], + related_task_id: UUID | str | None, +) -> bool: + """True iff every recipient already holds the dedup key (a re-fire). + + Per-recipient SET-NX: acquires (marks) keys for recipients NOT yet + notified this window, so the next fire converges toward full suppression. + Suppresses only when NO recipient was fresh (all already held). Fail-open: + a Redis error → False (never drop a notification over dedup infra). + """ + if ntype not in _LOOP_PRONE_TYPES: + return False + if from_agent is None or not recipients: + return False + + try: + conn = redis.from_url(settings.redis_url) + try: + any_fresh = False + for recipient in recipients: + acquired = await conn.set( + _key(ntype, from_agent, recipient, related_task_id), + "1", + nx=True, + ex=_DEDUP_TTL_SECONDS, + ) + if acquired: + any_fresh = True + return not any_fresh + finally: + await conn.aclose() + except Exception as exc: + logger.warning("notification dedup probe failed (redis): %s", exc) + return False diff --git a/roboco/services/notification_delivery.py b/roboco/services/notification_delivery.py index 32c14ad3..0f63fbe0 100644 --- a/roboco/services/notification_delivery.py +++ b/roboco/services/notification_delivery.py @@ -12,7 +12,7 @@ Also implements the ACK system for tracking acknowledgments. import asyncio from dataclasses import dataclass from datetime import UTC, datetime -from typing import ClassVar, Literal +from typing import ClassVar, Literal, cast from uuid import UUID import structlog @@ -29,6 +29,7 @@ from roboco.events import Event, EventType, get_event_bus from roboco.foundation.policy.communications import ACK_REQUIRED_BY_TYPE from roboco.models.base import AgentRole, NotificationPriority, NotificationType from roboco.services.base import BaseService, NotFoundError +from roboco.services.notification_dedup import all_recipients_recently_notified from roboco.utils.converters import require_uuid _log = structlog.get_logger(service="notification_delivery") @@ -873,6 +874,26 @@ class NotificationDeliveryService(BaseService): async def _persist_and_deliver(self, notification: NotificationTable) -> None: """Add to session, flush (to get an id), deliver. Caller commits.""" + # Re-fire guard (loop-prone types): this path skips the DB dedup, so + # apply the same 60s Redis SET-NX window. Fail-open on Redis down. + # Casts peel the SA UUID column type-leak for the type checker. + if await all_recipients_recently_notified( + ntype=notification.type, + from_agent=cast("UUID | None", notification.from_agent), + recipients=cast("list[UUID]", notification.to_agents), + related_task_id=cast("UUID | None", notification.related_task_id), + ): + _log.info( + "Suppressed re-fire notification (loop-prone, recent window)", + from_agent=str(notification.from_agent) + if notification.from_agent is not None + else None, + type=notification.type.value if notification.type is not None else None, + related_task_id=str(notification.related_task_id) + if notification.related_task_id is not None + else None, + ) + return self.session.add(notification) await self.session.flush() await self.deliver(require_uuid(notification.id)) diff --git a/roboco/services/sequencing.py b/roboco/services/sequencing.py index 2fa0aee9..9fc0ee9f 100644 --- a/roboco/services/sequencing.py +++ b/roboco/services/sequencing.py @@ -252,30 +252,61 @@ def dev_task_collision_edges(siblings: list) -> list[tuple[object, object]]: reverse edge (which would cycle). ``add_dependency`` dedupes, so repeated wiring is a no-op on already-wired pairs. """ + # Collision edges from DECLARED surfaces. Fewer than two surfaced siblings + # -> no collision path (edges stays empty); the undeclared-surface fallback + # below may still chain a same-assignee lane, so it must run regardless. surfaced = _surfaced_siblings(siblings) - if len(surfaced) < _MIN_COLLISION_PAIR: - return [] - # Stable order across incremental re-runs: priority is set at creation, - # sequence is append-only (existing siblings keep theirs). - surfaced.sort( - key=lambda s: (int(getattr(s, "priority", 2)), int(getattr(s, "sequence", 0))) - ) - surfaces = [ - DraftSurface( - idx=i, - priority=int(getattr(s, "priority", 2)), - intends_to_touch=list(getattr(s, "intends_to_touch", None) or []), - adds_migration=bool(getattr(s, "adds_migration", False)), - touches_shared=bool(getattr(s, "touches_shared", False)), - project_id=str(s.project_id) if s.project_id is not None else None, + edges: list[tuple[object, object]] = [] + if len(surfaced) >= _MIN_COLLISION_PAIR: + # Stable order across incremental re-runs: priority is set at creation, + # sequence is append-only (existing siblings keep theirs). + surfaced.sort( + key=lambda s: ( + int(getattr(s, "priority", 2)), + int(getattr(s, "sequence", 0)), + ) ) - for i, s in enumerate(surfaced) - ] - # cell_of / cell_capacity are advisory (contention warnings only); dev - # tasks under one cell-task share the parent's cell, so a constant keeps - # any warning attributable. Empty capacity -> no warnings emitted. - plan = SequencingService().analyze(surfaces, lambda _idx: "", {}) - return [(surfaced[a].id, surfaced[b].id) for a, b in plan.edges] + surfaces = [ + DraftSurface( + idx=i, + priority=int(getattr(s, "priority", 2)), + intends_to_touch=list(getattr(s, "intends_to_touch", None) or []), + adds_migration=bool(getattr(s, "adds_migration", False)), + touches_shared=bool(getattr(s, "touches_shared", False)), + project_id=str(s.project_id) if s.project_id is not None else None, + ) + for i, s in enumerate(surfaced) + ] + # cell_of / cell_capacity are advisory (contention warnings only); dev + # tasks under one cell-task share the parent's cell, so a constant keeps + # any warning attributable. Empty capacity -> no warnings emitted. + plan = SequencingService().analyze(surfaces, lambda _idx: "", {}) + edges = [(surfaced[a].id, surfaced[b].id) for a, b in plan.edges] + if edges: + return edges + + # Undeclared-surface fallback: same-assignee same-repo siblings share a + # working tree, so chain each (project, assignee) lane by (priority, + # sequence) to avoid an out-of-order merge conflict. Same-assignee scoped so + # cross-dev parallel work is untouched; the edge survives reassignment. + # Only fires with zero collision edges; same stable sort -> re-runs only add. + lanes: dict[tuple[str, object], list] = defaultdict(list) + for s in siblings: + proj = getattr(s, "project_id", None) + owner = getattr(s, "assigned_to", None) + if proj is not None and owner is not None: + lanes[(str(proj), owner)].append(s) + fallback: list[tuple[object, object]] = [] + for members in lanes.values(): + members.sort( + key=lambda s: ( + int(getattr(s, "priority", 2)), + int(getattr(s, "sequence", 0)), + ) + ) + for prev, cur in pairwise(members): + fallback.append((prev.id, cur.id)) + return fallback # --------------------------------------------------------------------------- diff --git a/roboco/services/task.py b/roboco/services/task.py index daaa4d49..6e0f36b4 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -8,6 +8,7 @@ Handles status transitions, assignments, and queries. import asyncio from dataclasses import dataclass from datetime import UTC, datetime, timedelta +from pathlib import Path from typing import TYPE_CHECKING, Any, ClassVar, cast from uuid import UUID, uuid4 @@ -1888,10 +1889,17 @@ class TaskService(BaseService): parent_branch=parent_branch, ) - branch_name, _ = await git_service.create_branch(workspace, team, request) - - task.branch_name = branch_name - await self.session.flush() + try: + branch_name, _ = await git_service.create_branch(workspace, team, request) + task.branch_name = branch_name + await self.session.flush() + except Exception: + # create_branch cuts a per-task worktree at + # {workspace}/.worktrees/{task-short}/; tear it down on failure so a + # claim retry doesn't collide with a stale worktree at that path + # (F123). Best-effort, no-op if the worktree was never created. + await self._remove_task_worktree(workspace, require_uuid(task.id)) + raise self.log.info( "Auto-created hierarchical branch", @@ -1902,6 +1910,21 @@ class TaskService(BaseService): ) return branch_name + async def _remove_task_worktree(self, clone_root: Path, task_id: UUID) -> None: + """Best-effort removal of a task's per-task worktree (F123 rollback).""" + from roboco.services.workspace import get_workspace_service + + worktree = clone_root / ".worktrees" / str(task_id)[:8] + try: + await get_workspace_service(self.session).remove_worktree( + clone_root, worktree + ) + except Exception: + self.log.warning( + "worktree cleanup on claim rollback failed", + task_id=str(task_id), + ) + async def _distinct_projects_for_task(self, task: TaskTable) -> list[UUID]: """The distinct projects a coordination root's map spans — one ``feature/main_pm/{root}`` integration branch each. @@ -4909,6 +4932,7 @@ class TaskService(BaseService): task, TaskStatus.COMPLETED, completing_agent_role or "cell_pm" ) await self._close_work_session_for_task(task, reason="task completed") + await self._remove_task_worktree_on_terminal(task) await self.session.flush() await self._trigger_completion_hooks(task, agent_id) @@ -5175,6 +5199,7 @@ class TaskService(BaseService): # Validate transition with CEO role requirement self._validate_and_set_status(task, TaskStatus.COMPLETED, "ceo") await self.session.flush() + await self._remove_task_worktree_on_terminal(task) # Extract learnings (fire-and-forget) bg_task = asyncio.create_task(self._extract_completion_learnings(task, None)) @@ -5519,10 +5544,14 @@ class TaskService(BaseService): await ws_service.abandon(require_uuid(task.work_session_id), reason=reason) async def _delete_task_branch_best_effort(self, task: TaskTable) -> None: - """Delete the task's remote branch on cancel. Never raises. + """Delete the task's remote branch + per-task worktree on cancel. - Skipped for tasks that didn't make it to a branch yet, or whose - PR already merged (merge path deletes the source branch). + Best-effort, never raises. Skipped for tasks that didn't make it to a + branch yet, or whose PR already merged (merge path deletes the source + branch). The worktree at ``{clone_root}/.worktrees/{task-short}/`` is + removed from the assignee's clone so cancelled tasks don't leak full + working trees on disk (F123). The stale-claim reaper must NOT call this + — it routes to ``pending`` for a re-claim that reuses the worktree. """ branch = task.branch_name if not branch: @@ -5538,6 +5567,7 @@ class TaskService(BaseService): git_service = get_git_service(self.session) await git_service.delete_task_branch(project_slug, str(branch)) + await self._remove_task_worktree_best_effort(task, project_slug) except Exception as e: # Cleanup is best-effort — don't fail the cancel if the # remote is unreachable or the branch is already gone. @@ -5548,6 +5578,54 @@ class TaskService(BaseService): error=str(e), ) + async def _remove_task_worktree_best_effort( + self, task: TaskTable, project_slug: str + ) -> None: + """Remove the per-task worktree from the assignee's clone. Never raises. + + No-op when the task has no resolvable assignee (pooled/unassigned at + cancel) or the assignee carries no team (can't form a clone path). + """ + assignee = task.assignee + if assignee is None or assignee.team is None or assignee.slug is None: + return + from roboco.services.workspace import get_workspace_service + + ws_service = get_workspace_service(self.session) + clone_root = ws_service.get_clone_root_path( + project_slug, assignee.team, assignee.slug + ) + worktree = clone_root / ".worktrees" / str(task.id)[:8] + await ws_service.remove_worktree(clone_root, worktree) + + async def _remove_task_worktree_on_terminal(self, task: TaskTable) -> None: + """Best-effort per-task worktree removal on terminal completion. + + Mirrors the cancel-path cleanup but WITHOUT deleting the remote branch + (the merge path already deleted it). A completed/merged task would + otherwise leak its worktree on disk until the whole agent is deleted + (F123). Best-effort: never raises, so a cleanup failure can't block + completion. No-op for branchless tasks (no worktree was ever cut). + Terminal-only by call site — earlier review states may bounce + ``needs_revision`` and need the worktree back. + """ + if not task.branch_name: + return + try: + result = await self.session.execute( + select(ProjectTable.slug).where(ProjectTable.id == task.project_id) + ) + project_slug = result.scalar_one_or_none() + if not project_slug: + return + await self._remove_task_worktree_best_effort(task, project_slug) + except Exception as e: + self.log.warning( + "Terminal worktree cleanup skipped", + task_id=str(task.id), + error=str(e), + ) + async def _close_work_session_for_task(self, task: TaskTable, reason: str) -> None: """Close the task's work session on successful completion. diff --git a/roboco/services/workspace.py b/roboco/services/workspace.py index 1f09fc3c..35d2b513 100644 --- a/roboco/services/workspace.py +++ b/roboco/services/workspace.py @@ -181,6 +181,42 @@ def _ensure_agent_owned(workspace: Path) -> None: ) +def _resolve_clone_root(workspace: Path) -> Path: + """The clone root for a workspace or one of its linked worktrees. + + ``.venv`` and ``.uv-python`` live at the clone root and are shared by every + worktree under ``{clone_root}/.worktrees/{id}/``. Given a worktree path, + return its clone root; given the clone root itself, return it unchanged. + Pure path logic keyed on the ``.worktrees`` layout from ``get_worktree_path`` + — no git call needed. + """ + if workspace.parent.name == ".worktrees": + return workspace.parent.parent + return workspace + + +def _uv_subprocess_env(workspace: Path) -> dict[str, str]: + """Env for a uv subprocess run by the orchestrator (root). + + Pins ``UV_PYTHON_INSTALL_DIR`` to ``/.uv-python`` so a non-system + Python (e.g. 3.14) uv fetches lands INSIDE the workspace bind mount — not in + ``/root/.local/share/uv/python`` (root-owned, ``/root`` is 0700, outside the + mount). The workspace ``.venv/bin/python`` then symlinks to an agent-owned + CPython on the shared volume, which ``_ensure_agent_owned`` chowns (``.uv-python`` + is not in ``_PRUNE_DIRS``), so the agent (uid 1000) can traverse it. Without + this every ``uv run`` died on ``Permission denied`` canonicalizing the venv + symlink (live be-dev-1 brick). Per-workspace → per-project isolation intact. + + Worktree-aware (F123): when the CWD is a per-task worktree, resolve up to the + clone root so the shared ``.uv-python`` is reused instead of re-fetching a + managed CPython per worktree. + """ + env = dict(os.environ) + clone_root = _resolve_clone_root(workspace) + env["UV_PYTHON_INSTALL_DIR"] = str(clone_root / ".uv-python") + return env + + # Thin wrapper around time.monotonic so tests can patch _monotonic without # affecting asyncio's own use of time.monotonic (which runs during event-loop # teardown and would exhaust a side_effect iterator if patched directly). @@ -394,6 +430,143 @@ class WorkspaceService: team_str = team.value if isinstance(team, Team) else str(team) return self.root / project_slug / team_str / agent_slug + def get_clone_root_path( + self, + project_slug: str, + team: Team | str, + agent_slug: str, + ) -> Path: + """The persistent clone root for an agent on a project. + + Same path as ``get_workspace_path`` (the real ``.git`` object store + + shared ``.venv`` / ``.uv-python`` live here). Named separately so the + worktree code can express clone-root vs per-task-worktree intent. + """ + return self.get_workspace_path(project_slug, team, agent_slug) + + def get_worktree_path( + self, + project_slug: str, + team: Team | str, + agent_slug: str, + task_short_id: str, + ) -> Path: + """Per-task working tree: ``{clone_root}/.worktrees/{task_short_id}``. + + Each task/branch gets its own checkout via ``git worktree add`` so a + coordinator PM holding multiple in_progress roots never clobbers one + root's working tree by checking out another's branch (F123). The clone + root (object store + venv) is shared underneath. + """ + if not task_short_id: + raise WorkspaceError( + f"Cannot resolve worktree path for {agent_slug}: " + "task_short_id is empty." + ) + clone_root = self.get_clone_root_path(project_slug, team, agent_slug) + return clone_root / ".worktrees" / task_short_id + + @staticmethod + def _worktree_git( + clone_root: Path, args: list[str], check: bool = True + ) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", "-C", str(clone_root), *args], + capture_output=True, + text=True, + check=check, + ) + + @staticmethod + def _link_shared_venv(worktree: Path, clone_root: Path) -> None: + """Symlink ``worktree/.venv -> ../../.venv`` (the clone-root venv). + + uv discovers ``.venv`` next to the worktree's ``pyproject.toml``; without + the symlink it re-syncs a fresh venv per worktree. The relative target + holds because every worktree sits at ``{clone_root}/.worktrees/{id}`` + (two levels deep). Idempotent: leaves an existing symlink/dir alone. + Only links once the clone-root venv exists — otherwise the symlink + dangles and uv errors or re-syncs a worktree-local venv that the + lexists guard then can't replace. install_dev_deps provisions + clone_root/.venv before the first worktree add on the fresh-claim path, + so a later ensure (resume) self-heals the link. + """ + link = worktree / ".venv" + if os.path.lexists(link): + return + if not (clone_root / ".venv").exists(): + return + worktree.mkdir(parents=True, exist_ok=True) + link.symlink_to("../../.venv") + + async def ensure_worktree( + self, clone_root: Path, worktree: Path, branch: str, base: str + ) -> None: + """Create the per-task linked worktree on ``branch`` from ``base``. + + Idempotent: a present, registered worktree is left in place (re-claim, + re-spawn). A new branch uses ``git worktree add -b ``; an + already-existing branch (re-claim after rollback) reuses it with + ``worktree add ``. Then symlinks the shared clone-root venv and + chowns BOTH the worktree and the clone root (shared ``.git/worktrees`` / + ``.venv`` / ``.uv-python``). F123: replaces the shared-clone + ``reset --hard`` + ``checkout -b`` that clobbered a still-active root. + """ + if not (worktree.exists() and (worktree / ".git").is_file()): + branch_exists = ( + self._worktree_git( + clone_root, + ["rev-parse", "--verify", "--quiet", f"refs/heads/{branch}"], + check=False, + ).returncode + == 0 + ) + if branch_exists: + add_args = ["worktree", "add", str(worktree), branch] + else: + add_args = ["worktree", "add", str(worktree), "-b", branch, base] + res = self._worktree_git(clone_root, add_args, check=False) + if res.returncode != 0: + raise WorkspaceError( + f"git worktree add failed for {branch}: {res.stderr.strip()}" + ) + self._link_shared_venv(worktree, clone_root) + await asyncio.to_thread(_ensure_agent_owned, worktree) + await asyncio.to_thread(_ensure_agent_owned, clone_root) + + async def ensure_worktree_for_resume( + self, clone_root: Path, worktree: Path, branch: str + ) -> None: + """Re-add a pruned/evicted worktree on resume (no ``-b`` — branch exists). + + Committed work survives in the branch ref; only the working tree was + removed (reaper / cancel / disk pressure). Idempotent: a present + worktree is a no-op. + """ + if not (worktree.exists() and (worktree / ".git").is_file()): + res = self._worktree_git( + clone_root, ["worktree", "add", str(worktree), branch], check=False + ) + if res.returncode != 0: + raise WorkspaceError( + f"git worktree re-add failed for {branch}: {res.stderr.strip()}" + ) + self._link_shared_venv(worktree, clone_root) + await asyncio.to_thread(_ensure_agent_owned, worktree) + await asyncio.to_thread(_ensure_agent_owned, clone_root) + + async def remove_worktree(self, clone_root: Path, worktree: Path) -> None: + """Remove a per-task worktree (cancel / terminal / reaper evict). + + Best-effort ``git worktree remove --force`` then ``prune`` so no dangling + admin dir collides with a future re-claim. No-op if the worktree is + already gone. + """ + self._worktree_git( + clone_root, ["worktree", "remove", "--force", str(worktree)], check=False + ) + self._worktree_git(clone_root, ["worktree", "prune"], check=False) + async def resolve_workspace( self, project_slug: str, @@ -1177,6 +1350,7 @@ class WorkspaceService: return subprocess.run( argv, cwd=str(workspace), + env=_uv_subprocess_env(workspace), capture_output=True, text=True, timeout=settings.workspace_dep_install_timeout_seconds, @@ -1233,6 +1407,7 @@ class WorkspaceService: return subprocess.run( argv, cwd=str(workspace), + env=_uv_subprocess_env(workspace), capture_output=True, text=True, timeout=settings.workspace_dep_install_timeout_seconds, diff --git a/tests/integration/test_task_service_transitions.py b/tests/integration/test_task_service_transitions.py index 5aa7b57f..4c8cc158 100644 --- a/tests/integration/test_task_service_transitions.py +++ b/tests/integration/test_task_service_transitions.py @@ -776,6 +776,75 @@ async def test_wire_sibling_collision_dag_serializes_overlapping_dev_tasks( assert t2.id not in r3.dependency_ids +@pytest.mark.asyncio +async def test_wire_sibling_collision_dag_chains_undeclared_same_assignee_lane( + task_setup: dict, db_session: AsyncSession +) -> None: + """Undeclared-surface fallback: two dev siblings with NO collision surface + on the same assignee + same repo are chained by sequence so the later one + waits for the earlier — the live out-of-order start (a dev with an unmerged + earlier task starting the next one) is prevented at wiring time. Cross-dev + siblings stay parallel (the lane is same-assignee scoped).""" + svc = task_setup["svc"] + parent = await svc.create(_req(task_setup)) + await db_session.flush() + other_dev = AgentTable( + id=uuid4(), + name="Dev2", + slug=f"be-dev-{uuid4().hex[:8]}", + role=AgentRole.DEVELOPER, + team=Team.BACKEND, + status=AgentStatus.ACTIVE, + model_config={}, + system_prompt="dev", + capabilities=[], + permissions={}, + metrics={}, + ) + db_session.add(other_dev) + await db_session.flush() + + async def _dev(seq: int, assignee: UUID) -> Any: + t = await svc.create_subtask( + TaskCreateRequest( + title=f"dev-{seq}", + description=f"dev task {seq} description long enough", + acceptance_criteria=["ac"], + team=Team.BACKEND, + created_by=task_setup["agent_id"], + project_id=task_setup["project_id"], + parent_task_id=parent.id, + task_type=TaskType.CODE, + nature=TaskNature.TECHNICAL, + estimated_complexity=Complexity.MEDIUM, + sequence=seq, + assigned_to=assignee, + ) + ) + await svc.set_sequence(t.id, seq) + return t + + # Same assignee, no declared surface -> fallback chains seq-1 behind seq-0. + a = await _dev(0, task_setup["agent_id"]) + b = await _dev(1, task_setup["agent_id"]) + # Different assignee, no declared surface -> parallel (no fallback edge). + c = await _dev(2, cast("UUID", other_dev.id)) + + await svc.wire_sibling_collision_dag(parent.id) + + ra = await svc.get(a.id) + rb = await svc.get(b.id) + rc = await svc.get(c.id) + assert ra is not None and rb is not None and rc is not None + # Earlier sibling leads the lane (no incoming edge). + assert ra.dependency_ids == [] + # Later same-assignee sibling waits on the earlier one. + assert a.id in rb.dependency_ids + # Cross-dev sibling is not chained onto the first dev's lane. + assert a.id not in rc.dependency_ids + assert b.id not in rc.dependency_ids + + @pytest.mark.asyncio async def test_wire_cell_task_wave_chain_chains_to_predecessor_cell_tasks( task_setup: dict, db_session: AsyncSession diff --git a/tests/unit/gateway/test_choreographer_lane_barrier.py b/tests/unit/gateway/test_choreographer_lane_barrier.py new file mode 100644 index 00000000..8122950b --- /dev/null +++ b/tests/unit/gateway/test_choreographer_lane_barrier.py @@ -0,0 +1,190 @@ +"""Per-dev lane barrier on the give_me_work -> claim path. + +A developer with a pre-delegated sequenced code queue must not start a later +code leaf while an earlier same-assignee sibling is still open: that is the +out-of-order start that wedged the merge (a later PR cut from a base that +predates the earlier sibling's unmerged changes). ``i_am_idle`` already drops +lane-held leaves via ``_pending_not_lane_held``; this locks the same barrier +on ``give_me_work``'s pre-assigned path and on ``_run_claim_guards`` (the +direct claim verb), so neither route can jump the queue. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, MagicMock +from uuid import UUID, uuid4 + +import pytest +from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps + + +def _make_deps(task: AsyncMock) -> ChoreographerDeps: + return ChoreographerDeps( + task=task, + work_session=AsyncMock(), + git=AsyncMock(), + a2a=AsyncMock(), + journal=AsyncMock(), + audit=AsyncMock(), + evidence_repo=AsyncMock(), + ) + + +def _dev_agent_task_svc() -> tuple[AsyncMock, UUID]: + task_svc = AsyncMock() + task_svc.agent_for.return_value = MagicMock(role="developer") + task_svc.list_pending_for_agent.return_value = [] + task_svc.list_assigned_for_agent.return_value = [] + task_svc.list_paused_for_agent.return_value = [] + task_svc.list_in_progress_for_agent.return_value = [] + # Default: lane clear (no earlier incomplete sibling). + task_svc.has_earlier_incomplete_code_sibling.return_value = False + return task_svc, uuid4() + + +# --------------------------------------------------------------------------- +# give_me_work: pre-assigned path must drop a lane-held code leaf +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_give_me_work_skips_lane_held_pre_assigned_dev_task() -> None: + """A pre-assigned pending code leaf sitting behind an earlier open + same-assignee sibling is dropped (not offered); with nothing else + available the dev goes idle rather than jumping its queue.""" + task_svc, agent_id = _dev_agent_task_svc() + leaf = MagicMock(id=uuid4(), status="pending", title="later-leaf") + task_svc.list_pending_for_agent.return_value = [leaf] + task_svc.has_earlier_incomplete_code_sibling.return_value = True + deps = _make_deps(task_svc) + c = Choreographer(deps) + + env = await c.give_me_work(agent_id) + body = env.as_dict() + assert body["status"] == "idle" + assert body["task_id"] is None + task_svc.has_earlier_incomplete_code_sibling.assert_awaited_once_with(leaf) + + +@pytest.mark.asyncio +async def test_give_me_work_offers_pre_assigned_when_lane_clear() -> None: + """A pre-assigned code leaf whose lane is clear (no earlier open sibling) + is offered as normal — the filter only drops positively lane-held leaves.""" + task_svc, agent_id = _dev_agent_task_svc() + leaf = MagicMock(id=uuid4(), status="pending", title="ready-leaf") + task_svc.list_pending_for_agent.return_value = [leaf] + task_svc.has_earlier_incomplete_code_sibling.return_value = False + deps = _make_deps(task_svc) + c = Choreographer(deps) + + env = await c.give_me_work(agent_id) + body = env.as_dict() + assert body["task_id"] == str(leaf.id) + + +@pytest.mark.asyncio +async def test_give_me_work_lane_filter_inert_under_partial_mock() -> None: + """An AsyncMock stub returns a truthy non-bool (not ``True``); ``is not + True`` keeps the filter inert so a partial test mock never drops a leaf + it cannot positively confirm is lane-held.""" + task_svc, agent_id = _dev_agent_task_svc() + leaf = MagicMock(id=uuid4(), status="pending", title="maybe-leaf") + task_svc.list_pending_for_agent.return_value = [leaf] + # Truthy stub, NOT the literal bool True -> inert (leaf kept). + task_svc.has_earlier_incomplete_code_sibling.return_value = MagicMock() + deps = _make_deps(task_svc) + c = Choreographer(deps) + + env = await c.give_me_work(agent_id) + body = env.as_dict() + assert body["task_id"] == str(leaf.id) + + +# --------------------------------------------------------------------------- +# _run_claim_guards: a direct claim of a lane-held code task is refused +# --------------------------------------------------------------------------- + + +def _claim_task( + *, task_type: str = "code", dependency_ids: list[Any] | None = None +) -> Any: + return MagicMock( + id=uuid4(), + status="pending", + assigned_to=uuid4(), + parent_task_id=uuid4(), + task_type=task_type, + dependency_ids=dependency_ids or [], + team="backend", + ) + + +@pytest.mark.asyncio +async def test_claim_guard_blocks_lane_held_code_task() -> None: + """A direct claim of a code leaf with an earlier open same-assignee + sibling is refused (invalid_state) and parked back to pending.""" + task_svc, agent_id = _dev_agent_task_svc() + task = _claim_task() + task_svc.get.return_value = task + task_svc.has_earlier_incomplete_code_sibling.return_value = True + deps = _make_deps(task_svc) + c = Choreographer(deps) + + guard = await c._run_claim_guards( + agent_id=agent_id, task=task, role_str="developer" + ) + assert guard is not None + assert guard.error == "invalid_state" + task_svc.release_dependency_blocked_claim.assert_awaited_once_with(task.id) + + +@pytest.mark.asyncio +async def test_claim_guard_allows_when_lane_clear() -> None: + """A code leaf whose lane is clear proceeds (no rejection).""" + task_svc, agent_id = _dev_agent_task_svc() + task = _claim_task() + task_svc.get.return_value = task + task_svc.has_earlier_incomplete_code_sibling.return_value = False + deps = _make_deps(task_svc) + c = Choreographer(deps) + + guard = await c._run_claim_guards( + agent_id=agent_id, task=task, role_str="developer" + ) + assert guard is None + task_svc.release_dependency_blocked_claim.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_claim_guard_fail_closed_on_lookup_error() -> None: + """If the lane lookup raises, the claim is refused (fail-closed) rather + than letting an out-of-order start through on a DB hiccup.""" + task_svc, agent_id = _dev_agent_task_svc() + task = _claim_task() + task_svc.get.return_value = task + task_svc.has_earlier_incomplete_code_sibling.side_effect = RuntimeError("db down") + deps = _make_deps(task_svc) + c = Choreographer(deps) + + guard = await c._run_claim_guards( + agent_id=agent_id, task=task, role_str="developer" + ) + assert guard is not None + assert guard.error == "invalid_state" + + +@pytest.mark.asyncio +async def test_claim_guard_lane_inert_for_non_code_task() -> None: + """A non-code task (e.g. planning) is not lane-ordered; even if the + predicate were to return True the guard must not block a coordinator's + non-code claim — the lane is code-only. Predicate False -> proceed.""" + task_svc, agent_id = _dev_agent_task_svc() + task = _claim_task(task_type="planning") + task_svc.get.return_value = task + task_svc.has_earlier_incomplete_code_sibling.return_value = False + deps = _make_deps(task_svc) + c = Choreographer(deps) + + guard = await c._run_claim_guards(agent_id=agent_id, task=task, role_str="main_pm") + assert guard is None diff --git a/tests/unit/runtime/test_respawn_persistence.py b/tests/unit/runtime/test_respawn_persistence.py index df6a871a..ad785e4e 100644 --- a/tests/unit/runtime/test_respawn_persistence.py +++ b/tests/unit/runtime/test_respawn_persistence.py @@ -64,7 +64,26 @@ def test_partition_keeps_live_nonterminal_rows() -> None: ) assert stale == [] assert restored[("be-pm", str(tid))]["count"] == _SEEDED_COUNT - assert restored[("be-pm", str(tid))]["last_status"] == "blocked" + # last_status is re-stamped to the LIVE status (a pre-restart status is as + # stale w.r.t. post-restart reality as last_check, which F034 already + # re-stamps). Otherwise a status mismatch across the restart gap disarms + # the breaker on the first post-restart spawn and re-burns the budget. + assert restored[("be-pm", str(tid))]["last_status"] == "in_progress" + + +def test_partition_restamps_last_status_to_live_not_stale() -> None: + # A restored row whose persisted last_status differs from the live status + # must take the LIVE status — the mismatch is a restart artifact, not + # evidence the wedge cleared. count is preserved either way. + tid = uuid4() + rows = [_row(tid, count=3, last_status="blocked")] + restored, stale = AgentOrchestrator._partition_respawn_rows( + rows, {tid: "in_progress"} + ) + assert stale == [] + entry = restored[("be-pm", str(tid))] + assert entry["count"] == _SEEDED_COUNT + assert entry["last_status"] == "in_progress" def test_partition_drops_terminal_and_missing_rows() -> None: @@ -333,6 +352,47 @@ async def test_restored_counter_trips_at_persisted_threshold_not_from_one() -> N assert orch._pm_respawn_tracker[("be-pm", task_id)]["count"] == _TRIP_COUNT +@pytest.mark.asyncio +async def test_restore_status_mismatch_does_not_reburn_threshold() -> None: + """A status mismatch across a restart gap must NOT disarm the breaker. + + The persisted row's last_status (pre-restart) can differ from the live + status without the wedge having cleared (a reaper/external transition in + the gap). If restore left the stale last_status, the first post-restart + spawn would see the mismatch, reset count to 1, and re-burn the whole + strike threshold against the still-wedged task — exactly the re-burn the + respawn_tracker table was built to prevent. Restore re-stamps last_status + to the live status, so the breaker fires at the persisted threshold. + """ + orch = _new_orchestrator() + cast("Any", orch)._schedule_respawn_persist = MagicMock() + task_id = uuid4() + factory, _db = _mock_session_factory( + [_row(task_id, count=3, last_status="blocked")], + [SimpleNamespace(id=task_id, status="in_progress")], + ) + with patch("roboco.db.base.get_session_factory", return_value=factory): + await orch.restore_respawn_tracker() + # Restore re-stamped last_status to the live "in_progress". + assert ( + orch._pm_respawn_tracker[("be-pm", str(task_id))]["last_status"] + == "in_progress" + ) + task = {"id": str(task_id), "status": "in_progress"} + fake_audit = AsyncMock() + fake_audit.has_recent_tracing_gap = AsyncMock(return_value=False) + with ( + patch("roboco.services.audit.get_audit_service", return_value=fake_audit), + patch( + "roboco.services.notification.NotificationService", + return_value=AsyncMock(), + ), + ): + gated = await orch._pm_respawn_should_gate("be-pm", task) + assert gated is True # count 3 -> 4 trips; NOT reset to 1 by the mismatch + assert orch._pm_respawn_tracker[("be-pm", str(task_id))]["count"] == _TRIP_COUNT + + @pytest.mark.asyncio async def test_restart_midloop_continues_identically_to_no_restart() -> None: """Transparency: the gate decision depends only on the dict contents. diff --git a/tests/unit/runtime/test_spawn_cwd_worktree.py b/tests/unit/runtime/test_spawn_cwd_worktree.py new file mode 100644 index 00000000..905a3cb2 --- /dev/null +++ b/tests/unit/runtime/test_spawn_cwd_worktree.py @@ -0,0 +1,230 @@ +"""Spawn-side per-task worktree wiring (F123, Phase B — the atomic counterpart). + +``create_branch`` now cuts a worktree at ``{clone_root}/.worktrees/{task-short}/`` +instead of checking the branch out on the shared clone. The agent must be +POINTED at that worktree or it edits the clone root (parked on the default +branch) on the wrong branch. This pins: ``SpawnGitContext`` carries +``task_short_id``; ``_task_git_context`` populates it (branchless roots get +none); and the container ``-w`` + Edit/Write allowlist move to the worktree +IN LOCKSTEP (one formula) when a task short id is present, falling back to the +clone root otherwise. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import patch + +from roboco.models.runtime import OrchestratorAgentConfig, SpawnGitContext +from roboco.runtime.orchestrator import ( + AgentOrchestrator, + _agent_cwd_path, + _agent_worktree_path, +) + + +def _make_dev_config( + *, + project_slug: str = "roboco-api", + task_short_id: str | None = None, + branch_name: str | None = "feature/backend/TASK0001", +) -> OrchestratorAgentConfig: + return OrchestratorAgentConfig( + agent_id="be-dev-1", + blueprint_path=Path("/app/agents/blueprints/be-dev-1.md"), + model="sonnet", + mcp_config_path=Path("/app/mcp-config.json"), + git_context=SpawnGitContext( + project_slug=project_slug, + branch_name=branch_name, + task_short_id=task_short_id, + ), + ) + + +def _minimal_hosts() -> dict[str, str | None]: + return { + "claude": "/home/runner/.claude", + "blueprints": "/app/agents/blueprints", + "docs": "/app/docs", + "workspaces": "/data/workspaces", + "mcp_config": "/app/mcp-config.json", + "prompt": "/app/system-prompt.md", + "settings": None, + "briefing": None, + } + + +def _mock_settings() -> dict[str, object]: + return { + "agent_tool_call_warn": 80, + "agent_tool_call_halt": 100, + "agent_loop_threshold": 5, + "agent_loop_window": 10, + "agent_stop_attempt_allowance": 2, + "manifest_host_dir": "/tmp/manifests", + "workspaces_root": "/data/workspaces", + } + + +def _build_cmd(config: OrchestratorAgentConfig) -> list[str]: + hosts = _minimal_hosts() + attrs = _mock_settings() + with ( + patch("roboco.runtime.orchestrator.settings") as mock_settings, + patch("roboco.runtime.orchestrator.Path.exists", return_value=False), + patch( + "roboco.runtime.orchestrator._build_manifest_for_agent", + return_value=None, + ), + ): + for k, v in attrs.items(): + setattr(mock_settings, k, v) + return AgentOrchestrator._build_mount_args( + "roboco-agent-be-dev-1", config, hosts + ) + + +def _workdir(cmd: list[str]) -> str | None: + if "-w" not in cmd: + return None + return cmd[cmd.index("-w") + 1] + + +def _make_minimal_orchestrator() -> AgentOrchestrator: + with patch.object(AgentOrchestrator, "__init__", return_value=None): + return AgentOrchestrator.__new__(AgentOrchestrator) + + +class TestSpawnGitContextTaskShortId: + def test_task_short_id_defaults_none(self) -> None: + ctx = SpawnGitContext(project_slug="p", branch_name="b") + assert ctx.task_short_id is None + + def test_task_short_id_round_trips(self) -> None: + ctx = SpawnGitContext( + project_slug="p", branch_name="b", task_short_id="a3c40fe7" + ) + assert ctx.task_short_id == "a3c40fe7" + + +class TestAgentWorktreePath: + def test_appends_worktrees_segment(self) -> None: + assert ( + _agent_worktree_path("roboco-api", "backend", "be-dev-1", "a3c40fe7") + == "/data/workspaces/roboco-api/backend/be-dev-1/.worktrees/a3c40fe7" + ) + + +class TestAgentCwdPath: + def test_worktree_when_task_short_id_set(self) -> None: + ctx = SpawnGitContext( + project_slug="roboco-api", + branch_name="feature/backend/TASK0001", + task_short_id="a3c40fe7", + ) + assert _agent_cwd_path("roboco-api", "backend", "be-dev-1", ctx) == ( + "/data/workspaces/roboco-api/backend/be-dev-1/.worktrees/a3c40fe7" + ) + + def test_clone_root_when_no_task_short_id(self) -> None: + ctx = SpawnGitContext( + project_slug="roboco-api", branch_name="feature/backend/TASK0001" + ) + assert _agent_cwd_path("roboco-api", "backend", "be-dev-1", ctx) == ( + "/data/workspaces/roboco-api/backend/be-dev-1" + ) + + def test_clone_root_when_no_git_context(self) -> None: + assert _agent_cwd_path("roboco-api", "backend", "be-dev-1", None) == ( + "/data/workspaces/roboco-api/backend/be-dev-1" + ) + + +class TestAppendWorkspaceCwdWorktree: + def test_workdir_is_worktree_when_task_short_id_set(self) -> None: + config = _make_dev_config(task_short_id="a3c40fe7") + cmd = _build_cmd(config) + wd = _workdir(cmd) + assert wd == ( + "/data/workspaces/roboco-api/backend/be-dev-1/.worktrees/a3c40fe7" + ) + + def test_workdir_is_clone_root_when_no_task_short_id(self) -> None: + config = _make_dev_config(task_short_id=None) + cmd = _build_cmd(config) + wd = _workdir(cmd) + assert wd == "/data/workspaces/roboco-api/backend/be-dev-1" + + +class TestCwdMatchesEditAllowlistWorktree: + """-w and the Edit/Write allowlist prefix must be the SAME path (lockstep).""" + + def test_worktree_path_matches_allowlist_prefix(self) -> None: + project_slug = "roboco-api" + cwd = _agent_cwd_path( + project_slug, + "backend", + "be-dev-1", + SpawnGitContext( + project_slug=project_slug, + branch_name="feature/backend/TASK0001", + task_short_id="a3c40fe7", + ), + ) + cell = f"/data/workspaces/{project_slug}/backend" + + orch = _make_minimal_orchestrator() + permissions = orch._get_role_permissions( + role="developer", workspace_path=cwd, cell_workspace_path=cell + ) + + # The Edit allow rule is Edit(///**); strip the leading slash + # added by _get_role_permissions to compare against cwd. + edit_rules = [r for r in permissions["allow"] if r.startswith("Edit(//")] + assert edit_rules, f"no Edit(//...) allow rule: {permissions['allow']}" + rule_path = edit_rules[0][len("Edit(/") : -4] # drop "Edit(/" and "/**)" + assert rule_path == cwd, ( + f"Edit allowlist prefix '{rule_path}' != cwd '{cwd}'; the -w flag " + "and the Edit/Write scope must point at the same worktree path." + ) + + # And the docker -w must equal the same cwd. + config = _make_dev_config(task_short_id="a3c40fe7") + cmd = _build_cmd(config) + assert _workdir(cmd) == cwd + + +class TestTaskGitContextTaskShortId: + def _orch(self) -> AgentOrchestrator: + return _make_minimal_orchestrator() + + def test_populates_task_short_id_when_branch_present(self) -> None: + orch = self._orch() + task_id = "a3c40fe7-0000-0000-0000-000000000000" + ctx = orch._task_git_context( + { + "project_slug": "roboco-api", + "branch_name": "feature/backend/abc12345", + "id": task_id, + } + ) + assert ctx is not None + assert ctx.task_short_id == "a3c40fe7" + assert ctx.branch_name == "feature/backend/abc12345" + + def test_no_task_short_id_for_branchless_root(self) -> None: + # A branchless coordination root (umbrella / no-project product root) + # has no worktree — task_short_id must stay None so the spawn cwd + # falls back to the clone root, not a phantom .worktrees/ dir. + orch = self._orch() + ctx = orch._task_git_context( + {"project_slug": "roboco-api", "branch_name": None, "id": "abc12345"} + ) + assert ctx is not None + assert ctx.task_short_id is None + + def test_returns_none_without_project_slug(self) -> None: + orch = self._orch() + ctx = orch._task_git_context({"branch_name": "b", "id": "abc12345"}) + assert ctx is None diff --git a/tests/unit/runtime/test_spawn_worktree_ensure.py b/tests/unit/runtime/test_spawn_worktree_ensure.py new file mode 100644 index 00000000..8751bc9c --- /dev/null +++ b/tests/unit/runtime/test_spawn_worktree_ensure.py @@ -0,0 +1,171 @@ +"""Spawn-time worktree ensure (F123, Phase B). + +A respawn re-points the container ``-w`` at the task's worktree. If the +worktree was pruned while the agent was down, ``docker run -w `` starts +the agent in a non-existent directory and its first command fails. So the +worktree must be re-attached (idempotent) BEFORE the container launches. +``_ensure_worktree_before_spawn`` is the chokepoint; it is a no-op for +branchless / no-task spawns (no worktree). +""" + +from __future__ import annotations + +from contextlib import asynccontextmanager +from pathlib import Path +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import uuid4 + +import pytest +from roboco.models.runtime import SpawnGitContext +from roboco.runtime.orchestrator import AgentOrchestrator, AgentReadinessError +from roboco.services.workspace import WorkspaceError + + +def _make_orchestrator() -> AgentOrchestrator: + orch = AgentOrchestrator.__new__(AgentOrchestrator) + orch._bg_tasks = set() + orch._running = True + return orch + + +@asynccontextmanager +async def _fake_db_ctx(db: Any) -> Any: + yield db + + +@pytest.mark.asyncio +async def test_ensures_worktree_when_task_short_id_set() -> None: + orch = _make_orchestrator() + ctx = SpawnGitContext( + project_slug="roboco-api", + branch_name="feature/backend/abc12345", + task_short_id="a3c40fe7", + ) + db = MagicMock() + ws = MagicMock() + ws.ensure_worktree_for_resume = AsyncMock() + + with ( + patch("roboco.db.base.get_db_context", return_value=_fake_db_ctx(db)), + patch("roboco.services.workspace.WorkspaceService", return_value=ws), + ): + await orch._ensure_worktree_before_spawn( + ctx, "roboco-api", "backend", "be-dev-1", "task-1" + ) + + ws.ensure_worktree_for_resume.assert_awaited_once() + args = ws.ensure_worktree_for_resume.call_args.args + assert args[0] == Path("/data/workspaces/roboco-api/backend/be-dev-1") + assert args[1] == Path( + "/data/workspaces/roboco-api/backend/be-dev-1/.worktrees/a3c40fe7" + ) + assert args[2] == "feature/backend/abc12345" + + +@pytest.mark.asyncio +async def test_noop_when_no_task_short_id() -> None: + # A branchless / no-task spawn has no worktree — must not touch the FS. + orch = _make_orchestrator() + ctx = SpawnGitContext(project_slug="roboco-api", branch_name=None) + + db = MagicMock() + ws = MagicMock() + with ( + patch("roboco.db.base.get_db_context", return_value=_fake_db_ctx(db)), + patch("roboco.services.workspace.WorkspaceService", return_value=ws), + ): + await orch._ensure_worktree_before_spawn( + ctx, "roboco-api", "backend", "be-dev-1", "task-1" + ) + + ws.ensure_worktree_for_resume.assert_not_called() + + +@pytest.mark.asyncio +async def test_fatal_failure_releases_claim_and_aborts() -> None: + # A FATAL git-state failure (WorkspaceError — the branch ref is gone, so the + # worktree cannot be re-added) must NOT launch the container at a missing + # -w path. It releases the claim (so the next claim rebuilds the worktree + # via create_branch) and aborts the spawn with AgentReadinessError. + orch = _make_orchestrator() + release = AsyncMock() + object.__setattr__(orch, "_release_claim_to_pending", release) + task_id = str(uuid4()) + ctx = SpawnGitContext( + project_slug="roboco-api", + branch_name="feature/backend/abc12345", + task_short_id="a3c40fe7", + ) + ws = MagicMock() + ws.ensure_worktree_for_resume = MagicMock( + side_effect=WorkspaceError("git worktree re-add failed") + ) + + with ( + patch("roboco.db.base.get_db_context", return_value=_fake_db_ctx(MagicMock())), + patch("roboco.services.workspace.WorkspaceService", return_value=ws), + pytest.raises(AgentReadinessError, match="worktree ensure failed"), + ): + await orch._ensure_worktree_before_spawn( + ctx, "roboco-api", "backend", "be-dev-1", task_id + ) + + release.assert_awaited_once_with(task_id) + + +@pytest.mark.asyncio +async def test_transient_failure_aborts_without_release() -> None: + # A TRANSIENT failure (DB hiccup / other) must still abort (don't launch at + # a possibly-missing path) but must NOT release the claim — a fresh claim + # would not help and re-cloning is destructive. Next tick retries the same + # claim. + orch = _make_orchestrator() + release = AsyncMock() + object.__setattr__(orch, "_release_claim_to_pending", release) + task_id = str(uuid4()) + ctx = SpawnGitContext( + project_slug="roboco-api", + branch_name="feature/backend/abc12345", + task_short_id="a3c40fe7", + ) + ws = MagicMock() + ws.ensure_worktree_for_resume = MagicMock(side_effect=RuntimeError("db down")) + + with ( + patch("roboco.db.base.get_db_context", return_value=_fake_db_ctx(MagicMock())), + patch("roboco.services.workspace.WorkspaceService", return_value=ws), + pytest.raises(AgentReadinessError, match="transient"), + ): + await orch._ensure_worktree_before_spawn( + ctx, "roboco-api", "backend", "be-dev-1", task_id + ) + + release.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_recoverable_ensure_no_raise_no_release() -> None: + # The happy / recoverable path (worktree present, or pruned-but-re-added + # from the surviving branch ref) must stay a silent no-op — that is the + # F123 Phase B happy path. No raise, no claim release. + orch = _make_orchestrator() + release = AsyncMock() + object.__setattr__(orch, "_release_claim_to_pending", release) + ctx = SpawnGitContext( + project_slug="roboco-api", + branch_name="feature/backend/abc12345", + task_short_id="a3c40fe7", + ) + ws = MagicMock() + ws.ensure_worktree_for_resume = AsyncMock() # succeeds + + with ( + patch("roboco.db.base.get_db_context", return_value=_fake_db_ctx(MagicMock())), + patch("roboco.services.workspace.WorkspaceService", return_value=ws), + ): + await orch._ensure_worktree_before_spawn( + ctx, "roboco-api", "backend", "be-dev-1", str(uuid4()) + ) + + release.assert_not_awaited() diff --git a/tests/unit/services/test_git.py b/tests/unit/services/test_git.py index b637000e..5e8fda9d 100644 --- a/tests/unit/services/test_git.py +++ b/tests/unit/services/test_git.py @@ -13,7 +13,6 @@ from unittest.mock import AsyncMock, MagicMock, patch from uuid import uuid4 import pytest -from roboco.api.schemas.git import GitCreateBranchRequest from roboco.config import settings from roboco.exceptions import GitCommandError, GitError from roboco.services.base import NotFoundError, UnauthorizedError @@ -600,6 +599,7 @@ async def test_commit_uses_longer_timeout_for_staging_and_commit() -> None: svc = _service() _bind(svc, "_workspace_for_branch", AsyncMock(return_value=Path("/tmp/ws"))) _bind(svc, "_assert_on_task_branch", AsyncMock()) + _bind(svc, "_ensure_worktree_for_commit", AsyncMock()) _bind(svc, "_task_for_branch", AsyncMock(return_value=None)) _bind(svc, "_parse_commit_stats", MagicMock(return_value=(1, 0, 1))) @@ -634,136 +634,6 @@ async def test_commit_uses_longer_timeout_for_staging_and_commit() -> None: assert timeouts_by_subcmd["log"] is None -@pytest.mark.asyncio -async def test_create_branch_idempotent_when_branch_already_exists() -> None: - # A prior attempt may have created the branch on disk before the DB recorded - # branch_name; `checkout -b` then fails 128. create_branch must switch to the - # existing branch instead of raising (the raise triggered a retry cascade). - branch = "feature/backend/abc12345--def67890" - svc = _service() - object.__setattr__(svc, "_resolve_base_branch", AsyncMock(return_value="master")) - object.__setattr__(svc, "_project_default_branch", AsyncMock(return_value="master")) - object.__setattr__(svc, "_token_for_project", AsyncMock(return_value=None)) - object.__setattr__( - svc, "_checkout_base_with_fallback", AsyncMock(return_value="master") - ) - - calls: list[list[str]] = [] - - async def fake_run_git( - _workspace: object, args: list[str], **_kw: object - ) -> object: - calls.append(list(args)) - rc = 1 if list(args[:2]) == ["checkout", "-b"] else 0 - return MagicMock(stdout="", returncode=rc) - - object.__setattr__(svc, "_run_git", fake_run_git) - - with ( - patch("roboco.services.git.build_branch_name", AsyncMock(return_value=branch)), - patch( - "roboco.services.git.get_task_service", - MagicMock(return_value=MagicMock(update=AsyncMock())), - ), - ): - await svc.create_branch( - Path("/tmp/ws"), - "backend", - GitCreateBranchRequest( - project_slug="roboco-api", - task_id=uuid4(), - branch_type="feature", - parent_branch=None, - ), - ) - - assert ["checkout", "-b", branch] in calls, "checkout -b attempted" - assert ["checkout", branch] in calls, "fell back to existing branch on 128" - - -def _create_branch_stubs(svc: GitService) -> None: - object.__setattr__(svc, "_resolve_base_branch", AsyncMock(return_value="master")) - object.__setattr__(svc, "_project_default_branch", AsyncMock(return_value="master")) - object.__setattr__(svc, "_token_for_project", AsyncMock(return_value=None)) - object.__setattr__( - svc, "_checkout_base_with_fallback", AsyncMock(return_value="master") - ) - - -async def _run_create_branch_with_existing_branch( - svc: GitService, branch: str, unique_commits: str -) -> list[list[str]]: - """Drive create_branch where `checkout -b` fails (branch exists) and the - branch has `unique_commits` commits of its own. Returns the git argv calls. - """ - calls: list[list[str]] = [] - - async def fake_run_git( - _workspace: object, args: list[str], **_kw: object - ) -> object: - calls.append(list(args)) - if list(args[:2]) == ["checkout", "-b"]: - return MagicMock(stdout="", returncode=1) # branch already exists - if list(args[:2]) == ["rev-list", "--count"]: - return MagicMock(stdout=f"{unique_commits}\n", returncode=0) - return MagicMock(stdout="", returncode=0) - - object.__setattr__(svc, "_run_git", fake_run_git) - with ( - patch("roboco.services.git.build_branch_name", AsyncMock(return_value=branch)), - patch( - "roboco.services.git.get_task_service", - MagicMock(return_value=MagicMock(update=AsyncMock())), - ), - ): - await svc.create_branch( - Path("/tmp/ws"), - "frontend", - GitCreateBranchRequest( - project_slug="roboco-panel", - task_id=uuid4(), - branch_type="feature", - parent_branch=None, - ), - ) - return calls - - -@pytest.mark.asyncio -async def test_create_branch_refreshes_no_work_existing_branch_to_base() -> None: - """An existing branch with no commits of its own is re-pointed at the fresh - base — a dependency-blocked task re-claimed after its upstream merged must - not keep building on the stale snapshot.""" - svc = _service() - _create_branch_stubs(svc) - calls = await _run_create_branch_with_existing_branch( - svc, "feature/frontend/abc12345--def67890", unique_commits="0" - ) - assert ["reset", "--hard", "master"] in calls, ( - "a no-work existing branch must be reset onto the fresh base" - ) - - -@pytest.mark.asyncio -async def test_create_branch_keeps_existing_branch_that_has_work() -> None: - """An existing branch carrying its own commits is NOT reset (work preserved).""" - svc = _service() - _create_branch_stubs(svc) - calls = await _run_create_branch_with_existing_branch( - svc, "feature/frontend/abc12345--def67890", unique_commits="3" - ) - # The fresh-claim tree-clean (a BARE `reset --hard`) is expected — it discards - # only uncommitted cruft from a prior task in the shared clone, never commits. - assert ["reset", "--hard"] in calls - # But the RE-POINT reset (`reset --hard `, which throws commits away) - # must NEVER fire for a branch carrying its own work. - # `c[2:]` truthy == there is a ref arg after "reset --hard" → it re-points. - repoint_resets = [c for c in calls if c[:2] == ["reset", "--hard"] and c[2:]] - assert not repoint_resets, ( - "a branch with real work must never be re-pointed onto base" - ) - - @pytest.mark.asyncio async def test_push_restates_gh001_as_permanent() -> None: """A >100MB push rejection (GH001) is re-raised with a clear, permanent diff --git a/tests/unit/services/test_git_commit_worktree.py b/tests/unit/services/test_git_commit_worktree.py new file mode 100644 index 00000000..cda0d6e2 --- /dev/null +++ b/tests/unit/services/test_git_commit_worktree.py @@ -0,0 +1,151 @@ +"""commit paths run inside the per-task worktree, not the shared clone (F123, Phase B). + +``create_branch`` cuts a worktree at ``{clone_root}/.worktrees/{task-short}/``; +the agent's container cwd is pointed there at spawn. The commit paths must +follow — ``commit_for_task`` and the gateway ``commit`` resolve the worktree +from the task id, ensure it is present (re-add if pruned), and run +``git add``/``git commit`` with the worktree as cwd. A commit on the clone +root would land on whatever branch the shared checkout is parked on (the +F123 clobber, on the write side). +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import UUID, uuid4 + +import pytest +from roboco.api.schemas.git import GitCommitRequest +from roboco.services.git import GitService + + +def _service() -> GitService: + svc = GitService.__new__(GitService) + svc.log = MagicMock() + svc.session = MagicMock() + return svc + + +def _req(task_id: UUID | None) -> GitCommitRequest: + return GitCommitRequest( + project_slug="roboco-api", + task_id=task_id, + message="implement the dashboard layout and routing", + commit_type="feat", + scope="panel", + body=None, + files=None, + ) + + +@pytest.mark.asyncio +async def test_commit_for_task_runs_git_in_worktree_not_clone() -> None: + svc = _service() + task_id = uuid4() + short = str(task_id)[:8] + clone = Path("/tmp/ws") + worktree = clone / ".worktrees" / short + + task = MagicMock(branch_name="feature/backend/abc12345", id=task_id) + object.__setattr__( + svc, "_assert_task_owned_with_branch", AsyncMock(return_value=task) + ) + object.__setattr__(svc, "get_workspace", AsyncMock(return_value=clone)) + object.__setattr__(svc, "_assert_on_task_branch", AsyncMock()) + object.__setattr__(svc, "_link_commit_to_task", AsyncMock()) + + captured: list[Path] = [] + + async def _capture_workspace(workspace: Path, *_a: object, **_k: object) -> tuple: + captured.append(Path(workspace)) + return ("deadbeef", "msg", 1, 1, 0) + + object.__setattr__(svc, "create_commit", AsyncMock(side_effect=_capture_workspace)) + + ws_svc = MagicMock() + ws_svc.ensure_worktree_for_resume = AsyncMock() + with patch( + "roboco.services.git.get_workspace_service", MagicMock(return_value=ws_svc) + ): + await svc.commit_for_task(uuid4(), _req(task_id)) + + assert captured, "create_commit must be called" + assert captured[0] == worktree, ( + f"commit must run in the worktree {worktree}, not the clone root; " + f"got {captured[0]}" + ) + ws_svc.ensure_worktree_for_resume.assert_awaited_once() + call = ws_svc.ensure_worktree_for_resume.await_args + assert call.args[0] == clone + assert call.args[1] == worktree + assert call.args[2] == "feature/backend/abc12345" + + +@pytest.mark.asyncio +async def test_commit_for_task_without_task_id_stays_on_clone_root() -> None: + # A no-task commit (task_id=None) has no worktree — it stays on the clone + # root, the existing behaviour, and must NOT call ensure_worktree_for_resume. + svc = _service() + clone = Path("/tmp/ws") + object.__setattr__(svc, "get_workspace", AsyncMock(return_value=clone)) + + captured: list[Path] = [] + + async def _capture_workspace(workspace: Path, *_a: object, **_k: object) -> tuple: + captured.append(Path(workspace)) + return ("deadbeef", "msg", 1, 1, 0) + + object.__setattr__(svc, "create_commit", AsyncMock(side_effect=_capture_workspace)) + + ws_svc = MagicMock() + ws_svc.ensure_worktree_for_resume = AsyncMock() + with patch( + "roboco.services.git.get_workspace_service", MagicMock(return_value=ws_svc) + ): + await svc.commit_for_task(uuid4(), _req(None)) + + assert captured[0] == clone + ws_svc.ensure_worktree_for_resume.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_gateway_commit_runs_git_in_worktree_not_clone() -> None: + svc = _service() + task_id = uuid4() + short = str(task_id)[:8] + clone = Path("/tmp/ws") + worktree = clone / ".worktrees" / short + + object.__setattr__(svc, "_workspace_for_branch", AsyncMock(return_value=clone)) + object.__setattr__(svc, "_assert_on_task_branch", AsyncMock()) + object.__setattr__(svc, "_task_for_branch", AsyncMock(return_value=None)) + object.__setattr__(svc, "_parse_commit_stats", MagicMock(return_value=(1, 0, 1))) + + cwds: list[Path] = [] + + async def _run_git(workspace: Path, args: list[str], **_kw: object) -> object: + cwds.append(Path(workspace)) + if args[:2] == ["log", "-1"]: + return MagicMock(stdout="deadbeef|feat: x\n", returncode=0) + return MagicMock(stdout="", returncode=0) + + object.__setattr__(svc, "_run_git", AsyncMock(side_effect=_run_git)) + + ws_svc = MagicMock() + ws_svc.ensure_worktree_for_resume = AsyncMock() + with patch( + "roboco.services.git.get_workspace_service", MagicMock(return_value=ws_svc) + ): + out = await svc.commit( + branch_name="feature/backend/abc12345", + message="implement the dashboard layout and routing", + task_id=task_id, + ) + + assert out["sha"] == "deadbeef" + assert cwds, "git ops must run" + assert all(c == worktree for c in cwds), ( + f"all gateway-commit git ops must run in the worktree {worktree}; got {cwds}" + ) + ws_svc.ensure_worktree_for_resume.assert_awaited_once() diff --git a/tests/unit/services/test_git_create_branch_worktree.py b/tests/unit/services/test_git_create_branch_worktree.py new file mode 100644 index 00000000..a2ad4205 --- /dev/null +++ b/tests/unit/services/test_git_create_branch_worktree.py @@ -0,0 +1,163 @@ +"""create_branch cuts a per-task worktree, not a shared-clone checkout (F123, Phase B). + +The old flow ``reset --hard`` + ``checkout `` + ``merge --ff-only`` + +``checkout -b`` ran on the ONE shared clone — so a coordinator PM claiming a +second root clobbered the first root's working tree. The new flow delegates to +``WorkspaceService.ensure_worktree`` (``git worktree add`` under +``{clone_root}/.worktrees/{task-short}/``) and pushes from the clone root. The +shared clone's HEAD is never moved by a claim. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import UUID, uuid4 + +import pytest +from roboco.api.schemas.git import GitCreateBranchRequest +from roboco.services.git import GitService + + +def _service() -> GitService: + svc = GitService.__new__(GitService) + svc.log = MagicMock() + svc.session = MagicMock() + return svc + + +def _stub_base(svc: GitService) -> None: + object.__setattr__(svc, "_resolve_base_branch", AsyncMock(return_value="master")) + object.__setattr__(svc, "_project_default_branch", AsyncMock(return_value="master")) + object.__setattr__(svc, "_token_for_project", AsyncMock(return_value=None)) + + +def _req(task_id: UUID) -> GitCreateBranchRequest: + return GitCreateBranchRequest( + project_slug="roboco-api", + task_id=task_id, + branch_type="feature", + parent_branch=None, + ) + + +async def _drive( + svc: GitService, task_id: UUID, unique_commits: str +) -> tuple[object, list[tuple[Path, list[str]]], list[tuple], str]: + """Run create_branch recording (_run_git calls, ensure_worktree calls).""" + calls: list[tuple[Path, list[str]]] = [] + + async def fake_run_git(workspace: Path, args: list[str], **_kw: object) -> object: + calls.append((Path(workspace), list(args))) + if args[:2] == ["rev-list", "--count"]: + return MagicMock(stdout=f"{unique_commits}\n", returncode=0) + # ls-remote / rev-parse / fetch / push all "succeed". + return MagicMock(stdout="abc\trefs/heads/master\n", returncode=0) + + object.__setattr__(svc, "_run_git", fake_run_git) + + ensure_calls: list[tuple] = [] + ws_svc = MagicMock() + ws_svc.ensure_worktree = AsyncMock( + side_effect=lambda clone_root, worktree, branch, base: ensure_calls.append( + (Path(clone_root), Path(worktree), branch, base) + ) + ) + + branch = "feature/backend/abc12345--def67890" + with ( + patch("roboco.services.git.build_branch_name", AsyncMock(return_value=branch)), + patch( + "roboco.services.git.get_task_service", + MagicMock(return_value=MagicMock(update=AsyncMock())), + ), + patch( + "roboco.services.git.get_workspace_service", MagicMock(return_value=ws_svc) + ), + ): + out = await svc.create_branch(Path("/tmp/ws"), "backend", _req(task_id)) + return out, calls, ensure_calls, branch + + +@pytest.mark.asyncio +async def test_create_branch_does_not_reset_or_checkout_shared_clone() -> None: + # THE F123 assertion: a claim never mutates the shared clone's working tree. + svc = _service() + _stub_base(svc) + _, calls, _ensure_calls, _branch = await _drive(svc, uuid4(), unique_commits="0") + + clone = Path("/tmp/ws") + bare_resets = [(ws, a) for ws, a in calls if a == ["reset", "--hard"]] + checkouts_on_clone = [ + (ws, a) for ws, a in calls if a[:1] == ["checkout"] and ws == clone + ] + assert not bare_resets, "shared-clone `reset --hard` clobber must not run" + assert not checkouts_on_clone, ( + "no checkout on the shared clone (worktree add replaces it)" + ) + + +@pytest.mark.asyncio +async def test_create_branch_calls_ensure_worktree_at_task_short_id_path() -> None: + svc = _service() + _stub_base(svc) + task_id = uuid4() + short = str(task_id)[:8] + _, _, ensure_calls, branch = await _drive(svc, task_id, unique_commits="0") + + assert ensure_calls, "ensure_worktree must be called" + clone_root, worktree, got_branch, base_ref = ensure_calls[0] + assert clone_root == Path("/tmp/ws") + assert worktree == Path("/tmp/ws") / ".worktrees" / short + assert got_branch == branch + # Bases off the fetched remote tip (origin/), matching the old + # `merge --ff-only origin/` intent. + assert base_ref == "origin/master" + + +@pytest.mark.asyncio +async def test_create_branch_pushes_branch_from_clone_root() -> None: + svc = _service() + _stub_base(svc) + _, calls, _, branch = await _drive(svc, uuid4(), unique_commits="0") + + pushes = [ + a for ws, a in calls if a[:3] == ["push", "-u", "origin"] and a[3] == branch + ] + assert pushes, "branch must be pushed from the clone root (shared refs)" + + +@pytest.mark.asyncio +async def test_create_branch_returns_branch_and_base_unchanged() -> None: + svc = _service() + _stub_base(svc) + out, _, _, branch = await _drive(svc, uuid4(), unique_commits="0") + assert out == (branch, "master") + + +@pytest.mark.asyncio +async def test_create_branch_repoints_empty_existing_branch_on_worktree_cwd() -> None: + # An existing branch with no commits of its own is re-pointed at the fresh + # base — but on the WORKTREE (not the shared clone), so a sibling root's + # tree is untouched. + svc = _service() + _stub_base(svc) + task_id = uuid4() + short = str(task_id)[:8] + _, calls, _, _ = await _drive(svc, task_id, unique_commits="0") + + repoints = [(ws, a) for ws, a in calls if a[:2] == ["reset", "--hard"] and a[2:]] + assert repoints, "an empty existing branch must be re-pointed to base" + assert repoints[0][0] == Path("/tmp/ws") / ".worktrees" / short, ( + "re-point must run on the worktree, not the shared clone" + ) + + +@pytest.mark.asyncio +async def test_create_branch_never_repoints_branch_with_real_work() -> None: + svc = _service() + _stub_base(svc) + _, calls, _, _ = await _drive(svc, uuid4(), unique_commits="3") + + repoints = [a for ws, a in calls if a[:2] == ["reset", "--hard"] and a[2:]] + assert not repoints, "a branch carrying real work must never be re-pointed" diff --git a/tests/unit/services/test_git_resolve_git_dir.py b/tests/unit/services/test_git_resolve_git_dir.py new file mode 100644 index 00000000..b3a53a57 --- /dev/null +++ b/tests/unit/services/test_git_resolve_git_dir.py @@ -0,0 +1,101 @@ +"""``resolve_git_dir`` — the worktree ``.git``-is-a-file helper (F123, Phase A). + +A linked worktree's ``.git`` is a *file* (a ``gitdir: `` pointer into the +clone root's ``.git/worktrees//``), not a directory. Every site that today +does ``workspace / ".git"`` and assumes a directory breaks under worktrees. This +helper is the single chokepoint that follows the pointer. +""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest +from roboco.services.git import resolve_git_dir + + +def _git(cwd: Path, *args: str) -> str: + env = { + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", + } + return subprocess.run( + ["git", "-C", str(cwd), *args], + check=True, + capture_output=True, + text=True, + env={**__import__("os").environ, **env}, + ).stdout + + +def _init_clone(clone: Path) -> None: + clone.mkdir(parents=True) + _git(clone, "init", "-b", "main") + (clone / "README.md").write_text("hi\n") + _git(clone, "add", "README.md") + _git(clone, "commit", "-m", "init") + + +@pytest.fixture +def clone(tmp_path: Path) -> Path: + c = tmp_path / "clone" + _init_clone(c) + return c + + +pytestmark = pytest.mark.skipif( + shutil.which("git") is None, reason="git CLI required for worktree tests" +) + + +def test_resolve_git_dir_clone_root_returns_dot_git_dir(clone: Path) -> None: + # The clone root's .git is a real directory. + resolved = resolve_git_dir(clone) + assert resolved == clone / ".git" + assert resolved.is_dir() + + +def test_resolve_git_dir_worktree_follows_gitdir_pointer(clone: Path) -> None: + # A linked worktree's .git is a FILE (gitdir pointer). The helper must + # follow it into clone/.git/worktrees//. + wt = clone / ".worktrees" / "t1" + _git(clone, "worktree", "add", str(wt), "-b", "feature/t1") + + assert (wt / ".git").is_file(), "linked worktree .git must be a file" + + resolved = resolve_git_dir(wt) + assert resolved is not None + # Points into the clone's worktree admin area, not the worktree's own .git file. + assert resolved.is_dir() + assert resolved.parent.parent == clone / ".git" + assert resolved.parent.name == "worktrees" + # Sanity: the gitdir file points here. + pointer = (wt / ".git").read_text().strip() + assert pointer.startswith("gitdir: ") + assert Path(pointer[len("gitdir: ") :].strip()) == resolved + + +def test_resolve_git_dir_no_git_returns_none(tmp_path: Path) -> None: + # A bare dir with no .git: callers (e.g. _remove_stale_git_locks) must get + # None and bail cleanly, not crash on a missing path. + bare = tmp_path / "no-repo" + bare.mkdir() + assert resolve_git_dir(bare) is None + + +def test_resolve_git_dir_worktree_locks_are_reachable(clone: Path) -> None: + # The motivating caller: _remove_stale_git_locks must be able to rglob + # *.lock inside a WORKTREE's git dir. Proves the pointer-follow resolves to + # a rglob-able directory. + wt = clone / ".worktrees" / "t1" + _git(clone, "worktree", "add", str(wt), "-b", "feature/t1") + resolved = resolve_git_dir(wt) + assert resolved is not None + (resolved / "index.lock").write_text("fake") + # rglob reaches it (this is what _remove_stale_git_locks will do). + locks = list(resolved.rglob("*.lock")) + assert any(p.name == "index.lock" for p in locks) diff --git a/tests/unit/services/test_git_worktree_routing_gaps.py b/tests/unit/services/test_git_worktree_routing_gaps.py new file mode 100644 index 00000000..0d513080 --- /dev/null +++ b/tests/unit/services/test_git_worktree_routing_gaps.py @@ -0,0 +1,178 @@ +"""Rebase + conventions validator run in the per-task worktree, not the clone. + +F123 gap: Phase B routed ``create_branch`` + ``commit`` to the worktree but +missed two cwd-dependent git ops that still resolved the clone root: + +1. ``rebase_onto_base`` (called by ``sync_task_branch`` + ``rebase_pr_for_task``) + does ``git checkout `` + ``git reset --hard origin/`` in the + resolved workspace. Post-F123 the branch is checked out in the linked + worktree, so a ``checkout`` in the clone root is refused ("already checked + out at ''") — the behind-base recovery loop + PM wedged-PR rebase + are dead on arrival. + +2. ``conventions_check_for_task`` runs the validator with ``--root ``; the validator reads ``(root/rel).read_bytes()`` — default-branch + content, not the dev's worktree changes. Newly-added files are absent from + the clone root → false pass; modified files are analyzed at stale content. + +Both fix the same way: resolve the worktree via ``_worktree_for_task(clone_root, +task.id)`` + ``_ensure_worktree_for_commit`` and run the op there. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock +from uuid import UUID, uuid4 + +import pytest +from roboco.services.git import GitService + + +def _service() -> GitService: + svc = GitService.__new__(GitService) + svc.log = MagicMock() + svc.session = MagicMock() + return svc + + +def _task(*, branch: str, task_id: UUID | None = None) -> MagicMock: + return MagicMock( + id=task_id or uuid4(), + project_id=uuid4(), + branch_name=branch, + assigned_to=uuid4(), + ) + + +# --- rebase: sync_task_branch + rebase_pr_for_task route to the worktree --- + + +def _stub_rebase_common(svc: GitService, clone: Path) -> dict[str, list[Path]]: + object.__setattr__( + svc, "_project_for_task", AsyncMock(return_value=MagicMock(slug="roboco-api")) + ) + object.__setattr__( + svc, "_resolve_workspace_agent_id", MagicMock(return_value=uuid4()) + ) + object.__setattr__(svc, "get_workspace", AsyncMock(return_value=clone)) + object.__setattr__( + svc, "_get_project_token_or_raise", AsyncMock(return_value="tok") + ) + object.__setattr__(svc, "_ensure_worktree_for_commit", AsyncMock()) + + cwds: list[Path] = [] + + async def _run_git(workspace: Path, args: list[str], **_kw: object) -> object: + cwds.append(Path(workspace)) + if args[:1] == ["rev-list"]: + return MagicMock(stdout="0\n", returncode=0) + return MagicMock(stdout="", returncode=0) + + object.__setattr__(svc, "_run_git", AsyncMock(side_effect=_run_git)) + return {"cwds": cwds} + + +@pytest.mark.asyncio +async def test_sync_task_branch_rebases_in_worktree_not_clone() -> None: + svc = _service() + task_id = uuid4() + short = str(task_id)[:8] + clone = Path("/tmp/ws") + worktree = clone / ".worktrees" / short + task = _task(branch="feature/backend/abc12345", task_id=task_id) + + state = _stub_rebase_common(svc, clone) + + await svc.sync_task_branch(task, base_branch="master") + + ensure = object.__getattribute__(svc, "_ensure_worktree_for_commit") + ensure.assert_awaited_once() + args = ensure.await_args.args + assert args[0] == clone, "ensure must target the clone root" + assert args[1] == worktree, ( + f"ensure must target the worktree {worktree}; got {args[1]}" + ) + assert args[2] == "feature/backend/abc12345" + assert state["cwds"], "rebase git ops must run" + assert all(c == worktree for c in state["cwds"]), ( + f"all rebase git ops must run in the worktree {worktree}; got {state['cwds']}" + ) + + +@pytest.mark.asyncio +async def test_rebase_pr_for_task_rebases_in_worktree_not_clone() -> None: + svc = _service() + task_id = uuid4() + short = str(task_id)[:8] + clone = Path("/tmp/ws") + worktree = clone / ".worktrees" / short + task = _task(branch="feature/backend/abc12345", task_id=task_id) + + state = _stub_rebase_common(svc, clone) + object.__setattr__( + svc, "_parse_github_remote", MagicMock(return_value=("owner", "repo")) + ) + object.__setattr__( + svc, + "_get_pr_refs", + AsyncMock(return_value=("feature/backend/abc12345", "master")), + ) + # rebase_pr_for_task loads the task from the DB by (pr_number, project_id). + session = MagicMock() + result = MagicMock() + result.scalar_one_or_none.return_value = task + session.execute = AsyncMock(return_value=result) + svc.session = session + + await svc.rebase_pr_for_task(pr_number=42, project_id=uuid4()) + + ensure = object.__getattribute__(svc, "_ensure_worktree_for_commit") + ensure.assert_awaited_once() + assert ensure.await_args.args[1] == worktree + assert state["cwds"], "rebase git ops must run" + assert all(c == worktree for c in state["cwds"]), ( + f"all rebase git ops must run in the worktree {worktree}; got {state['cwds']}" + ) + + +# --- conventions: validator --root points at the worktree, not the clone --- + + +@pytest.mark.asyncio +async def test_conventions_check_runs_validator_in_worktree_not_clone() -> None: + svc = _service() + task_id = uuid4() + short = str(task_id)[:8] + clone = Path("/tmp/ws") + worktree = clone / ".worktrees" / short + task = _task(branch="feature/backend/abc12345", task_id=task_id) + + object.__setattr__(svc, "_workspace_for_branch", AsyncMock(return_value=clone)) + object.__setattr__( + svc, "list_changed_files", AsyncMock(return_value=["src/foo.py"]) + ) + object.__setattr__(svc, "_ensure_worktree_for_commit", AsyncMock()) + + captured: list[Path] = [] + + async def _capture_validator( + workspace: Path, _files: list[str] + ) -> dict[str, object]: + captured.append(Path(workspace)) + return {"findings": [], "could_not_run": False} + + object.__setattr__( + svc, "_run_conventions_validator", AsyncMock(side_effect=_capture_validator) + ) + + await svc.conventions_check_for_task(actor_agent_id=uuid4(), task=task) + + ensure = object.__getattribute__(svc, "_ensure_worktree_for_commit") + ensure.assert_awaited_once() + assert ensure.await_args.args[1] == worktree + assert captured, "validator must run" + assert captured[0] == worktree, ( + f"validator --root must be the worktree {worktree}, not the clone root; " + f"got {captured[0]}" + ) diff --git a/tests/unit/services/test_sequencing.py b/tests/unit/services/test_sequencing.py index 09091fed..cfa5fd60 100644 --- a/tests/unit/services/test_sequencing.py +++ b/tests/unit/services/test_sequencing.py @@ -211,6 +211,7 @@ class _Sib: adds_migration: bool = False touches_shared: bool = False project_id: str | None = "proj-backend" + assigned_to: object | None = None def _edge_set(pairs: list[tuple[object, object]]) -> set[tuple[object, object]]: @@ -290,6 +291,69 @@ def test_dev_collision_returns_depends_on_first_pairs() -> None: assert task == second.id +# --------------------------------------------------------------------------- +# dev_task_collision_edges — undeclared-surface fallback: same-assignee +# same-repo siblings chain by (priority, sequence); cross-dev stays parallel. +# --------------------------------------------------------------------------- + + +def test_dev_collision_fallback_chains_same_assignee_no_surface() -> None: + # Same dev, same repo, no declared surface -> chain by sequence. + a = _Sib(uuid4(), sequence=0, assigned_to="be-dev-1") + b = _Sib(uuid4(), sequence=1, assigned_to="be-dev-1") + assert dev_task_collision_edges([a, b]) == [(a.id, b.id)] + + +def test_dev_collision_fallback_skips_cross_assignee() -> None: + # Two different devs on the same repo, no surface -> parallel. + a = _Sib(uuid4(), sequence=0, assigned_to="be-dev-1") + b = _Sib(uuid4(), sequence=1, assigned_to="be-dev-2") + assert dev_task_collision_edges([a, b]) == [] + + +def test_dev_collision_fallback_skips_unassigned() -> None: + # No assignee -> can't determine a per-dev lane -> skip. + a = _Sib(uuid4(), sequence=0) + b = _Sib(uuid4(), sequence=1) + assert dev_task_collision_edges([a, b]) == [] + + +def test_dev_collision_fallback_skips_different_project() -> None: + # Same dev, different repos -> no shared working tree -> no chain. + a = _Sib(uuid4(), sequence=0, assigned_to="be-dev-1", project_id="proj-be") + b = _Sib(uuid4(), sequence=1, assigned_to="be-dev-1", project_id="proj-fe") + assert dev_task_collision_edges([a, b]) == [] + + +def test_dev_collision_fallback_does_not_override_collision_edges() -> None: + # Declared overlapping surface -> collision edge wins; no fallback chain. + a = _Sib(uuid4(), sequence=0, assigned_to="be-dev-1", intends_to_touch=["a.py"]) + b = _Sib(uuid4(), sequence=1, assigned_to="be-dev-1", intends_to_touch=["a.py"]) + assert dev_task_collision_edges([a, b]) == [(a.id, b.id)] + + +def test_dev_collision_fallback_orders_by_priority_then_sequence() -> None: + # Mixed priority/sequence -> chain in (priority, sequence) ascending order. + p2s2 = _Sib(uuid4(), priority=2, sequence=2, assigned_to="be-dev-1") + p1s5 = _Sib(uuid4(), priority=1, sequence=5, assigned_to="be-dev-1") + p1s1 = _Sib(uuid4(), priority=1, sequence=1, assigned_to="be-dev-1") + edges = dev_task_collision_edges([p2s2, p1s5, p1s1]) # passed out of order + assert edges == [(p1s1.id, p1s5.id), (p1s5.id, p2s2.id)] + + +def test_dev_collision_fallback_single_sibling_no_edge() -> None: + # A chain needs >= 2 same-assignee same-project siblings. + solo = _Sib(uuid4(), sequence=0, assigned_to="be-dev-1") + assert dev_task_collision_edges([solo]) == [] + + +def test_dev_collision_fallback_idempotent_on_rerun() -> None: + # Deterministic sort -> two calls return the same edge list. + a = _Sib(uuid4(), sequence=0, assigned_to="be-dev-1") + b = _Sib(uuid4(), sequence=1, assigned_to="be-dev-1") + assert dev_task_collision_edges([a, b]) == dev_task_collision_edges([a, b]) + + # --------------------------------------------------------------------------- # cell_task_wave_chain_depends_on — the cell-task wave chain (edge kind 2). # Pure glue: a new cell-task under root-subtask UT_n depends on every cell-task diff --git a/tests/unit/services/test_task_cancel_worktree_cleanup.py b/tests/unit/services/test_task_cancel_worktree_cleanup.py new file mode 100644 index 00000000..94e001af --- /dev/null +++ b/tests/unit/services/test_task_cancel_worktree_cleanup.py @@ -0,0 +1,190 @@ +"""Cancel tears down the per-task worktree (F123, Phase C). + +``_delete_task_branch_best_effort`` already deletes the task's REMOTE branch on +cancel. Without also removing the local per-task worktree at +``{clone_root}/.worktrees/{task-short}/``, every cancelled task leaks a full +working tree on the assignee's clone — disk blowup (plan risk #6). The reaper +(stale-claim → pending) must NOT remove it (a re-claim reuses it); only the +terminal cancel path does. The assignee is joined-eager-loaded on the task and +``_abandon_work_session_for_task`` does not clear ``assigned_to``, so the +clone root is resolvable at the cancel hook. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import uuid4 + +import pytest +from roboco.models.base import Team +from roboco.services.task import TaskService + + +def _service() -> TaskService: + svc = TaskService.__new__(TaskService) + svc.log = MagicMock() + svc.session = MagicMock() + return svc + + +def _session(slug: str | None = "roboco-api") -> MagicMock: + # Build on a local MagicMock (sub-attr assignment is allowed there) then + # callers assign the whole thing to ``svc.session`` — assigning + # ``svc.session.execute`` directly trips mypy's method-assign on the typed + # AsyncSession attribute. + session = MagicMock() + session.execute = AsyncMock(return_value=_project_result(slug)) + return session + + +def _task(*, branch: str | None, assignee: MagicMock | None) -> MagicMock: + task_id = uuid4() + return MagicMock( + id=task_id, + project_id=uuid4(), + branch_name=branch, + assignee=assignee, + ) + + +def _project_result(slug: str | None) -> MagicMock: + result = MagicMock() + result.scalar_one_or_none.return_value = slug + return result + + +@pytest.mark.asyncio +async def test_cancel_removes_worktree_for_assignee() -> None: + svc = _service() + task = _task( + branch="feature/backend/abc12345", + assignee=MagicMock(slug="be-dev-1", team=Team.BACKEND), + ) + short = str(task.id)[:8] + clone = Path("/data/workspaces/roboco-api/backend/be-dev-1") + + svc.session = _session("roboco-api") + + git_service = MagicMock() + git_service.delete_task_branch = AsyncMock() + ws_svc = MagicMock() + ws_svc.get_clone_root_path = MagicMock(return_value=clone) + ws_svc.remove_worktree = AsyncMock() + + with ( + patch( + "roboco.services.git.get_git_service", + MagicMock(return_value=git_service), + ), + patch( + "roboco.services.workspace.get_workspace_service", + MagicMock(return_value=ws_svc), + ), + ): + await svc._delete_task_branch_best_effort(task) + + git_service.delete_task_branch.assert_awaited_once_with( + "roboco-api", "feature/backend/abc12345" + ) + ws_svc.get_clone_root_path.assert_called_once_with( + "roboco-api", Team.BACKEND, "be-dev-1" + ) + ws_svc.remove_worktree.assert_awaited_once() + args = ws_svc.remove_worktree.await_args.args + assert args[0] == clone, "remove must target the clone root" + assert args[1] == clone / ".worktrees" / short, ( + f"remove must target the task worktree {clone}/.worktrees/{short}; " + f"got {args[1]}" + ) + + +@pytest.mark.asyncio +async def test_cancel_skips_worktree_when_no_assignee() -> None: + # Unassigned at cancel time (e.g. pooled task cancelled before any claim) — + # no clone root to resolve, so the worktree step is skipped. The remote + # branch is still deleted. + svc = _service() + task = _task(branch="feature/backend/abc12345", assignee=None) + svc.session = _session("roboco-api") + + git_service = MagicMock() + git_service.delete_task_branch = AsyncMock() + ws_svc = MagicMock() + ws_svc.remove_worktree = AsyncMock() + + with ( + patch( + "roboco.services.git.get_git_service", + MagicMock(return_value=git_service), + ), + patch( + "roboco.services.workspace.get_workspace_service", + MagicMock(return_value=ws_svc), + ), + ): + await svc._delete_task_branch_best_effort(task) + + git_service.delete_task_branch.assert_awaited_once() + ws_svc.remove_worktree.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_cancel_skips_worktree_when_no_branch() -> None: + # Branchless coordination root — no worktree was ever created. + svc = _service() + task = _task(branch=None, assignee=MagicMock(slug="be-dev-1", team=Team.BACKEND)) + svc.session = _session(None) + + git_service = MagicMock() + git_service.delete_task_branch = AsyncMock() + ws_svc = MagicMock() + ws_svc.remove_worktree = AsyncMock() + + with ( + patch( + "roboco.services.git.get_git_service", + MagicMock(return_value=git_service), + ), + patch( + "roboco.services.workspace.get_workspace_service", + MagicMock(return_value=ws_svc), + ), + ): + await svc._delete_task_branch_best_effort(task) + + git_service.delete_task_branch.assert_not_awaited() + ws_svc.remove_worktree.assert_not_awaited() + svc.session.execute.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_worktree_cleanup_failure_does_not_raise() -> None: + # Best-effort: a remove_worktree failure (missing clone, git error) must not + # abort the cancel — the remote branch was already deleted. + svc = _service() + task = _task( + branch="feature/backend/abc12345", + assignee=MagicMock(slug="be-dev-1", team=Team.BACKEND), + ) + svc.session = _session("roboco-api") + + git_service = MagicMock() + git_service.delete_task_branch = AsyncMock() + ws_svc = MagicMock() + ws_svc.get_clone_root_path = MagicMock( + return_value=Path("/data/workspaces/roboco-api/backend/be-dev-1") + ) + ws_svc.remove_worktree = AsyncMock(side_effect=RuntimeError("boom")) + + with ( + patch( + "roboco.services.git.get_git_service", + MagicMock(return_value=git_service), + ), + patch( + "roboco.services.workspace.get_workspace_service", + MagicMock(return_value=ws_svc), + ), + ): + await svc._delete_task_branch_best_effort(task) # must not raise diff --git a/tests/unit/services/test_task_claim_rollback_worktree.py b/tests/unit/services/test_task_claim_rollback_worktree.py new file mode 100644 index 00000000..03f2289d --- /dev/null +++ b/tests/unit/services/test_task_claim_rollback_worktree.py @@ -0,0 +1,106 @@ +"""Claim-rollback tears down the per-task worktree (F123, Phase B). + +``_create_branch_in_project`` calls ``create_branch``, which cuts a worktree +at ``{clone_root}/.worktrees/{task-short}/``. If a step after the worktree-add +fails (the push, the branch_name flush), the worktree is orphaned at that path +— and a claim retry collides with the stale worktree (``git worktree add`` +refuses: "already exists"). The rollback removes it (best-effort, no-op if the +worktree was never created). +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import uuid4 + +import pytest +from roboco.services.task import TaskService + + +def _service() -> TaskService: + svc = TaskService.__new__(TaskService) + svc.log = MagicMock() + svc.session = MagicMock() + return svc + + +@pytest.mark.asyncio +async def test_create_branch_failure_removes_worktree() -> None: + svc = _service() + task_id = uuid4() + short = str(task_id)[:8] + clone = Path("/tmp/ws") + + task = MagicMock(id=task_id, project_id=uuid4(), branch_name=None) + project = MagicMock(slug="roboco-api") + + object.__setattr__(svc, "_resolve_parent_branch", AsyncMock(return_value=None)) + object.__setattr__(svc, "_resolve_team_dir", MagicMock(return_value="backend")) + + git_service = MagicMock() + git_service.get_workspace = AsyncMock(return_value=clone) + git_service.create_branch = AsyncMock(side_effect=RuntimeError("push failed")) + + ws_svc = MagicMock() + ws_svc.remove_worktree = AsyncMock() + + with ( + patch( + "roboco.services.git.get_git_service", MagicMock(return_value=git_service) + ), + patch( + "roboco.services.workspace.get_workspace_service", + MagicMock(return_value=ws_svc), + ), + pytest.raises(RuntimeError, match="push failed"), + ): + await svc._create_branch_in_project(task, uuid4(), project) + + ws_svc.remove_worktree.assert_awaited_once() + args = ws_svc.remove_worktree.await_args.args + assert args[0] == clone, "remove must target the clone root" + assert args[1] == clone / ".worktrees" / short, ( + f"remove must target the task worktree {clone}/.worktrees/{short}; " + f"got {args[1]}" + ) + + +@pytest.mark.asyncio +async def test_successful_create_does_not_remove_worktree() -> None: + svc = _service() + task_id = uuid4() + clone = Path("/tmp/ws") + + task = MagicMock(id=task_id, project_id=uuid4(), branch_name=None) + project = MagicMock(slug="roboco-api") + + object.__setattr__(svc, "_resolve_parent_branch", AsyncMock(return_value=None)) + object.__setattr__(svc, "_resolve_team_dir", MagicMock(return_value="backend")) + + git_service = MagicMock() + git_service.get_workspace = AsyncMock(return_value=clone) + git_service.create_branch = AsyncMock(return_value=("feature/x", "master")) + + ws_svc = MagicMock() + ws_svc.remove_worktree = AsyncMock() + + # Assign the whole session (not svc.session.flush directly) — mypy treats + # the typed AsyncSession.flush as a method and rejects the sub-assignment. + session = MagicMock() + session.flush = AsyncMock() + svc.session = session + + with ( + patch( + "roboco.services.git.get_git_service", MagicMock(return_value=git_service) + ), + patch( + "roboco.services.workspace.get_workspace_service", + MagicMock(return_value=ws_svc), + ), + ): + out = await svc._create_branch_in_project(task, uuid4(), project) + + assert out == "feature/x" + ws_svc.remove_worktree.assert_not_awaited() diff --git a/tests/unit/services/test_workspace_uv_python_install_dir.py b/tests/unit/services/test_workspace_uv_python_install_dir.py new file mode 100644 index 00000000..9578d702 --- /dev/null +++ b/tests/unit/services/test_workspace_uv_python_install_dir.py @@ -0,0 +1,202 @@ +"""Per-workspace ``UV_PYTHON_INSTALL_DIR`` — the workspace-venv brick cure (Fix 2). + +Root cause (live on be-dev-1, project requires Python 3.14): ``install_dev_deps`` +runs ``uv sync --python 3.14`` as ROOT in the orchestrator, so uv fetches the +managed CPython into its default ``/root/.local/share/uv/python`` (root-owned, +``/root`` is 0700). The workspace ``.venv/bin/python`` symlinks there, the +symlink target is OUTSIDE the workspace bind mount, and ``_ensure_agent_owned`` +can't chown it — so the agent (uid 1000) hits ``Permission denied (os error 13)`` +canonicalizing ``.venv/bin/python3`` and every ``uv run`` dies. Fix 1 (bash-guard) +protects the sacred ``/app/.venv`` but does NOT cure this. + +Cure: pin ``UV_PYTHON_INSTALL_DIR`` to ``/.uv-python`` so the managed +CPython the venv symlinks to lives INSIDE the workspace bind mount and is chowned +to the agent by the existing ``_ensure_agent_owned`` walk (``.uv-python`` is not +in ``_PRUNE_DIRS``). Per-workspace → per-project isolation intact (no global +shared interpreter). ``/app/.venv`` untouched. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path as _Path +from typing import TYPE_CHECKING +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from roboco.services.workspace import ( + _PRUNE_DIRS, + WorkspaceService, + _uv_subprocess_env, +) + +if TYPE_CHECKING: + from pathlib import Path + + +def _service() -> WorkspaceService: + session = MagicMock() + session.execute = AsyncMock() + return WorkspaceService(session) + + +def _make_workspace(tmp_path: Path) -> Path: + workspace = tmp_path / "roboco" / "backend" / "be-dev-1" + (workspace / ".git").mkdir(parents=True) + return workspace + + +# --------------------------------------------------------------------------- +# _PRUNE_DIRS — the chown walk must reach .uv-python +# --------------------------------------------------------------------------- + + +def test_uv_python_install_dir_not_pruned() -> None: + # If .uv-python were pruned, _ensure_agent_owned would never chown the + # managed CPython and the agent still couldn't traverse it. + assert ".uv-python" not in _PRUNE_DIRS + + +def test_repo_gitignore_ignores_uv_python_dir() -> None: + # The per-workspace managed-CPython dir lives inside the clone (and thus + # inside every worktree checkout). It must be gitignored so an agent never + # commits a multi-GB CPython fetch. + gitignore = _Path(__file__).resolve().parents[3] / ".gitignore" + assert gitignore.exists(), f".gitignore not found at {gitignore}" + lines = gitignore.read_text().splitlines() + assert ".uv-python/" in lines, ".uv-python/ must be gitignored (now per-workspace)" + + +def test_uv_subprocess_env_clone_root_when_cwd_is_worktree(tmp_path: Path) -> None: + # F123: a task's worktree is a separate checkout, but .venv / .uv-python stay + # at the CLONE root (shared). A uv run launched from a worktree CWD must still + # pin UV_PYTHON_INSTALL_DIR at the clone root's .uv-python — not a phantom + # /.uv-python — so the managed CPython is found and not re-fetched + # per worktree. + clone = tmp_path / "roboco" / "backend" / "be-dev-1" + worktree = clone / ".worktrees" / "a3c40fe7" + worktree.mkdir(parents=True) + + env = _uv_subprocess_env(worktree) + + assert env["UV_PYTHON_INSTALL_DIR"] == str(clone / ".uv-python") + + +def test_uv_subprocess_env_clone_root_unchanged_for_clone_itself( + tmp_path: Path, +) -> None: + # Regression guard: when the CWD IS the clone root (no .worktrees segment), + # behavior is byte-for-byte the pre-worktree path. + clone = tmp_path / "roboco" / "backend" / "be-dev-1" + clone.mkdir(parents=True) + + env = _uv_subprocess_env(clone) + + assert env["UV_PYTHON_INSTALL_DIR"] == str(clone / ".uv-python") + + +# --------------------------------------------------------------------------- +# _run_dep_install — uv must fetch the managed CPython into the workspace +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_run_dep_install_sets_uv_python_install_dir(tmp_path: Path) -> None: + ws = _make_workspace(tmp_path) + svc = _service() + captured_env: dict[str, str] = {} + + def _fake_run( + argv: list[str], *, env: dict[str, str] | None = None, **_kw: object + ) -> subprocess.CompletedProcess[str]: + if env is not None: + captured_env.update(env) + return subprocess.CompletedProcess(argv, returncode=0, stdout="", stderr="") + + with patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run): + await svc._run_dep_install( + ws, "uv sync --extra dev", ["uv", "sync", "--extra", "dev"] + ) + + assert "UV_PYTHON_INSTALL_DIR" in captured_env + # Must point INSIDE the workspace bind mount (the brick was the managed + # CPython landing in /root, outside the mount + root-owned). + assert captured_env["UV_PYTHON_INSTALL_DIR"] == str(ws / ".uv-python") + + +@pytest.mark.asyncio +async def test_toolchain_smoke_sets_uv_python_install_dir(tmp_path: Path) -> None: + # The smoke also runs `uv run --python ` and would otherwise fetch the + # managed CPython into /root a second time. + ws = _make_workspace(tmp_path) + svc = _service() + captured_env: dict[str, str] = {} + + def _fake_run( + argv: list[str], *, env: dict[str, str] | None = None, **_kw: object + ) -> subprocess.CompletedProcess[str]: + if env is not None: + captured_env.update(env) + return subprocess.CompletedProcess(argv, returncode=0, stdout="", stderr="") + + with patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run): + await svc._run_toolchain_smoke(ws, "3.14") + + assert captured_env.get("UV_PYTHON_INSTALL_DIR") == str(ws / ".uv-python") + + +@pytest.mark.asyncio +async def test_install_dev_deps_uv_python_dir_inside_workspace(tmp_path: Path) -> None: + # End-to-end: install_dev_deps runs uv with UV_PYTHON_INSTALL_DIR pointing + # inside the workspace, so the managed CPython is on the shared volume and + # gets chowned to the agent. Regression guard for the live be-dev-1 brick. + ws = _make_workspace(tmp_path) + (ws / "pyproject.toml").write_text("[project]\nname = 'x'\n") + (ws / "uv.lock").write_text("version = 1\n") + svc = _service() + captured_env: dict[str, str] = {} + + def _fake_run( + argv: list[str], *, env: dict[str, str] | None = None, **_kw: object + ) -> subprocess.CompletedProcess[str]: + if env is not None: + captured_env.update(env) + return subprocess.CompletedProcess(argv, returncode=0, stdout="", stderr="") + + with ( + patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run), + patch("roboco.services.workspace._ensure_agent_owned"), + ): + await svc.install_dev_deps(ws) + + assert captured_env.get("UV_PYTHON_INSTALL_DIR") == str(ws / ".uv-python") + + +@pytest.mark.asyncio +async def test_install_env_inherits_parent_environ(tmp_path: Path) -> None: + # The injected env must still carry PATH etc. (uv must be found) — we merge + # into os.environ, not replace it. + ws = _make_workspace(tmp_path) + svc = _service() + captured_env: dict[str, str] = {} + + def _fake_run( + argv: list[str], *, env: dict[str, str] | None = None, **_kw: object + ) -> subprocess.CompletedProcess[str]: + if env is not None: + captured_env.update(env) + return subprocess.CompletedProcess(argv, returncode=0, stdout="", stderr="") + + with ( + patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run), + patch.dict( + "os.environ", + {"PATH": "/usr/bin:/bin", "ROBOCO_TEST_MARKER": "1"}, + clear=False, + ), + ): + await svc._run_dep_install(ws, "uv sync", ["uv", "sync"]) + + assert captured_env.get("PATH") == "/usr/bin:/bin" + assert captured_env.get("ROBOCO_TEST_MARKER") == "1" + assert captured_env.get("UV_PYTHON_INSTALL_DIR") == str(ws / ".uv-python") diff --git a/tests/unit/services/test_workspace_uv_resolves_clone_venv.py b/tests/unit/services/test_workspace_uv_resolves_clone_venv.py new file mode 100644 index 00000000..a3402e2b --- /dev/null +++ b/tests/unit/services/test_workspace_uv_resolves_clone_venv.py @@ -0,0 +1,134 @@ +"""uv resolves the clone-root ``.venv`` from a per-task worktree (F123, risk #1). + +The highest unknown in the worktree design: uv discovers ``.venv`` next to the +worktree's ``pyproject.toml``. A worktree has no ``.venv`` of its own, so +without the ``worktree/.venv -> ../../.venv`` symlink uv would re-sync a fresh +venv per task (slow + divergent toolchains). This proves the symlink makes uv +resolve the shared clone-root venv when invoked from the worktree cwd — the +exact resolution path an agent's ``make quality`` hits. + +Real subprocesses (git + uv); skipped when ``uv`` is absent. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +from roboco.services.workspace import WorkspaceService + +pytestmark = pytest.mark.skipif( + shutil.which("uv") is None, reason="uv CLI not installed" +) + + +def _git(cwd: Path, *args: str) -> str: + env = { + **os.environ, + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", + } + return subprocess.run( + ["git", "-C", str(cwd), *args], + check=True, + capture_output=True, + text=True, + env=env, + ).stdout + + +def _run(cwd: Path, *cmd: str) -> str: + # Scrub the parent uv environment so uv discovers fresh from the worktree + # cwd instead of inheriting the test process's VIRTUAL_ENV (which would + # mask the worktree .venv symlink and false-pass/fail the resolution). + env = { + k: v + for k, v in os.environ.items() + if k not in {"VIRTUAL_ENV", "UV_PROJECT_ENVIRONMENT", "UV_PYTHON_INSTALL_DIR"} + } + return subprocess.run( + list(cmd), + check=True, + capture_output=True, + text=True, + cwd=str(cwd), + env=env, + ).stdout + + +@pytest.fixture +def clone(tmp_path: Path) -> Path: + c = tmp_path / "clone" + c.mkdir(parents=True) + _git(c, "init", "-b", "main") + (c / "pyproject.toml").write_text("[project]\nname = 'x'\nversion = '0'\n") + _git(c, "add", "pyproject.toml") + _git(c, "commit", "-m", "init") + # Real clone-root venv — the symlink target uv must resolve to. + _run(c, "uv", "venv", ".venv") + return c + + +def _service() -> WorkspaceService: + return WorkspaceService(MagicMock()) + + +def test_worktree_venv_symlink_points_at_clone_root(clone: Path) -> None: + svc = _service() + worktree = clone / ".worktrees" / "abc12345" + svc._link_shared_venv(worktree, clone) + + link = worktree / ".venv" + assert link.is_symlink(), "worktree/.venv must be a symlink" + assert link.readlink() == Path("../../.venv") + # Resolves to the clone-root venv, not a per-worktree one. + assert link.resolve() == (clone / ".venv").resolve() + + +async def test_uv_resolves_clone_root_venv_from_worktree(clone: Path) -> None: + svc = _service() + worktree = clone / ".worktrees" / "abc12345" + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, worktree, "feature/x", "main") + + # uv invoked from the worktree must use the clone-root venv's python, + # not create/sync a worktree-local one. Loads the worktree pyproject + # (which has no deps) and skips sync. + out = _run( + worktree, + "uv", + "run", + "--no-sync", + "python", + "-c", + "import sys; print(sys.executable)", + ).strip() + clone_venv_python = (clone / ".venv" / "bin" / "python").resolve() + assert Path(out).resolve() == clone_venv_python, ( + f"uv must resolve the clone-root venv from the worktree; " + f"got {out}, expected {clone_venv_python}" + ) + + +async def test_clone_root_stays_on_default_after_worktree_add(clone: Path) -> None: + # THE F123 assertion: cutting a task worktree does NOT move the clone root + # off the default branch. A second task's worktree is independent. + svc = _service() + wt1 = clone / ".worktrees" / "task1" + wt2 = clone / ".worktrees" / "task2" + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt1, "feature/a", "main") + await svc.ensure_worktree(clone, wt2, "feature/b", "main") + + clone_head = _git(clone, "branch", "--show-current").strip() + assert clone_head == "main", ( + f"clone root must stay on default after worktree add; got {clone_head}" + ) + assert _git(wt1, "branch", "--show-current").strip() == "feature/a" + assert _git(wt2, "branch", "--show-current").strip() == "feature/b" diff --git a/tests/unit/services/test_workspace_worktree_lifecycle.py b/tests/unit/services/test_workspace_worktree_lifecycle.py new file mode 100644 index 00000000..9ac0059b --- /dev/null +++ b/tests/unit/services/test_workspace_worktree_lifecycle.py @@ -0,0 +1,242 @@ +"""Per-task worktree lifecycle primitives (F123, Phase A — additive, not yet wired). + +``ensure_worktree`` / ``ensure_worktree_for_resume`` / ``remove_worktree`` on +WorkspaceService. These are the pure primitives Phase B's claim/resume flow will +call. Tested against a real tmp git clone — no DB, no Docker, no mocks of git. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from typing import TYPE_CHECKING +from unittest.mock import patch + +import pytest +from roboco.services.workspace import WorkspaceService + +if TYPE_CHECKING: + from pathlib import Path + + +def _git(cwd: Path, *args: str) -> str: + env = { + **os.environ, + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", + } + return subprocess.run( + ["git", "-C", str(cwd), *args], + check=True, + capture_output=True, + text=True, + env=env, + ).stdout + + +def _init_clone(clone: Path) -> None: + clone.mkdir(parents=True) + _git(clone, "init", "-b", "main") + (clone / "pyproject.toml").write_text("[project]\nname = 'x'\n") + _git(clone, "add", "pyproject.toml") + _git(clone, "commit", "-m", "init") + + +def _service() -> WorkspaceService: + return WorkspaceService( + __import__("unittest.mock", fromlist=["MagicMock"]).MagicMock() + ) + + +@pytest.fixture +def clone(tmp_path: Path) -> Path: + c = tmp_path / "clone" + _init_clone(c) + return c + + +pytestmark = pytest.mark.skipif( + shutil.which("git") is None, reason="git CLI required for worktree tests" +) + + +async def test_ensure_worktree_creates_linked_worktree_on_new_branch( + clone: Path, +) -> None: + svc = _service() + wt = clone / ".worktrees" / "a3c40fe7" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + + assert (wt / ".git").is_file(), "linked worktree .git must be a gitdir file" + assert _git(wt, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/a3c40fe7" + + +async def test_ensure_worktree_symlinks_venv_to_clone_root(clone: Path) -> None: + # uv discovers .venv next to pyproject.toml IN the worktree. Without a + # symlink to the clone-root .venv, uv re-syncs per worktree (bad). The + # symlink lets uv resolve the shared clone-root venv. + svc = _service() + (clone / ".venv").mkdir() # clone-root venv exists from install_dev_deps + wt = clone / ".worktrees" / "a3c40fe7" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + + venv_link = wt / ".venv" + assert venv_link.is_symlink(), ( + "worktree .venv must be a symlink to clone-root .venv" + ) + assert venv_link.resolve() == (clone / ".venv").resolve() + + +async def test_ensure_worktree_no_dangling_venv_symlink_when_clone_root_venv_missing( + clone: Path, +) -> None: + # If the clone-root venv is not yet provisioned, the worktree .venv symlink + # must NOT be created — a dangling ../../.venv symlink makes uv error or + # re-sync a worktree-local venv that the lexists guard then can't replace. + # install_dev_deps provisions clone_root/.venv before the first worktree + # add on the fresh-claim path, so this only fires in the near-zero gap. + svc = _service() + assert not (clone / ".venv").exists() + wt = clone / ".worktrees" / "a3c40fe7" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + + link = wt / ".venv" + assert not link.is_symlink(), ( + "no symlink when clone-root venv is absent (would dangle)" + ) + assert not link.exists() + + +async def test_ensure_worktree_links_venv_once_clone_root_venv_provisioned( + clone: Path, +) -> None: + # Self-heal: a worktree claimed before the clone-root venv existed gets no + # symlink; once install_dev_deps provisions clone_root/.venv, the next + # ensure (resume path) links it. + svc = _service() + wt = clone / ".worktrees" / "a3c40fe7" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + assert not (wt / ".venv").is_symlink() + + (clone / ".venv").mkdir() # install_dev_deps completes + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree_for_resume(clone, wt, "feature/a3c40fe7") + + link = wt / ".venv" + assert link.is_symlink() + assert link.resolve() == (clone / ".venv").resolve() + + +async def test_ensure_worktree_idempotent_on_existing_worktree(clone: Path) -> None: + svc = _service() + wt = clone / ".worktrees" / "a3c40fe7" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + # Second call must be a no-op, not an error ("already exists"). + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + + assert _git(wt, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/a3c40fe7" + + +async def test_ensure_worktree_chowns_both_worktree_and_clone_root(clone: Path) -> None: + # The two-target ownership invariant: the worktree working tree AND the + # clone root (shared .git/worktrees//, .venv, .uv-python) must be + # agent-owned. _ensure_agent_owned is mocked so we assert the CALL sites. + svc = _service() + wt = clone / ".worktrees" / "a3c40fe7" + owned: list[Path] = [] + + def _capture(p: Path) -> None: + owned.append(p) + + with patch("roboco.services.workspace._ensure_agent_owned", side_effect=_capture): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + + assert clone in owned, "clone root must be chowned (shared .venv/.git)" + assert wt in owned, "worktree working tree must be chowned" + + +async def test_ensure_worktree_for_resume_noop_when_present(clone: Path) -> None: + svc = _service() + wt = clone / ".worktrees" / "a3c40fe7" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + # Resume on an existing worktree: no-op, branch intact. + await svc.ensure_worktree_for_resume(clone, wt, "feature/a3c40fe7") + + assert _git(wt, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/a3c40fe7" + + +async def test_ensure_worktree_for_resume_readds_pruned_worktree(clone: Path) -> None: + # A pruned/evicted worktree must be re-added on resume (committed work + # survives in the branch ref). Re-add uses NO -b (branch already exists). + svc = _service() + wt = clone / ".worktrees" / "a3c40fe7" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + # Simulate eviction: remove the worktree out-of-band. + _git(clone, "worktree", "remove", str(wt), "--force") + assert not wt.exists() + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree_for_resume(clone, wt, "feature/a3c40fe7") + + assert wt.exists() + assert _git(wt, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/a3c40fe7" + + +async def test_remove_worktree_cleans_up_and_prunes(clone: Path) -> None: + svc = _service() + wt = clone / ".worktrees" / "a3c40fe7" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt, "feature/a3c40fe7", "main") + await svc.remove_worktree(clone, wt) + + assert not wt.exists(), "worktree dir must be gone" + listed = _git(clone, "worktree", "list", "--porcelain") + assert str(wt) not in listed, "worktree must be unregistered from clone" + + +async def test_remove_worktree_noop_on_missing_worktree(clone: Path) -> None: + # Cancel/reaper on a task whose worktree was never created (or already + # removed) must not raise. + svc = _service() + wt = clone / ".worktrees" / "never" + await svc.remove_worktree(clone, wt) # no error + assert not wt.exists() + + +async def test_two_concurrent_task_worktrees_independent(clone: Path) -> None: + # THE F123 assertion: two tasks of one PM get independent checkouts on the + # same clone, each on its own branch, neither clobbering the other. + svc = _service() + wt_a = clone / ".worktrees" / "a3c40fe7" + wt_b = clone / ".worktrees" / "8e460893" + + with patch("roboco.services.workspace._ensure_agent_owned"): + await svc.ensure_worktree(clone, wt_a, "feature/a3c40fe7", "main") + await svc.ensure_worktree(clone, wt_b, "feature/8e460893", "main") + + # Edit in worktree A does not appear in worktree B. + (wt_a / "new.txt").write_text("a") + assert (wt_a / "new.txt").exists() + assert not (wt_b / "new.txt").exists() + assert _git(wt_a, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/a3c40fe7" + assert _git(wt_b, "rev-parse", "--abbrev-ref", "HEAD").strip() == "feature/8e460893" + # Clone root stays on main — neither task branch moved it. + assert _git(clone, "rev-parse", "--abbrev-ref", "HEAD").strip() == "main" diff --git a/tests/unit/services/test_workspace_worktree_paths.py b/tests/unit/services/test_workspace_worktree_paths.py new file mode 100644 index 00000000..e086613e --- /dev/null +++ b/tests/unit/services/test_workspace_worktree_paths.py @@ -0,0 +1,82 @@ +"""Per-task git-worktree path model (F123 fix, Phase A prep). + +The coordinator PM exemption lets a PM hold multiple in_progress roots, but the +clone is one checkout — so switching roots' branches clobbers the working tree +(live on NAS: main-pm ping-ponged 03f80432 <-> c80e19ff on one clone). The fix: +each task gets its own working tree under ``{clone_root}/.worktrees/{task-short}/`` +via ``git worktree add``. These tests pin the path layout BEFORE the helpers are +wired into the claim/spawn flow (Phase B). +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, cast +from unittest.mock import AsyncMock, MagicMock + +import pytest +from roboco.models.base import Team +from roboco.services.workspace import WorkspaceError, WorkspaceService + +if TYPE_CHECKING: + from pathlib import Path + + +def _service(root: Path) -> WorkspaceService: + svc = WorkspaceService(MagicMock()) + svc.session = AsyncMock() + svc.root = root + return svc + + +def test_get_clone_root_path_equals_get_workspace_path(tmp_path: Path) -> None: + # The clone root IS the existing per-agent workspace path; the new helper + # is a named alias so call sites can express intent (clone-root vs worktree). + svc = _service(tmp_path) + clone = svc.get_clone_root_path("guard-core", Team.BACKEND, "be-dev-1") + assert clone == svc.get_workspace_path("guard-core", Team.BACKEND, "be-dev-1") + assert clone == tmp_path / "guard-core" / "backend" / "be-dev-1" + + +def test_get_worktree_path_lays_out_under_clone_root(tmp_path: Path) -> None: + svc = _service(tmp_path) + wt = svc.get_worktree_path("guard-core", Team.BACKEND, "be-dev-1", "a3c40fe7") + clone = svc.get_clone_root_path("guard-core", Team.BACKEND, "be-dev-1") + assert wt == clone / ".worktrees" / "a3c40fe7" + # And expressed from the workspaces root: + assert ( + wt + == tmp_path / "guard-core" / "backend" / "be-dev-1" / ".worktrees" / "a3c40fe7" + ) + + +def test_get_worktree_path_rejects_none_team(tmp_path: Path) -> None: + # Mirrors get_workspace_path's guard: a None team would produce a literal + # "None" segment and a broken path. + svc = _service(tmp_path) + with pytest.raises(WorkspaceError): + svc.get_worktree_path( + "guard-core", cast("Team | str", None), "be-dev-1", "a3c40fe7" + ) + + +def test_get_worktree_path_accepts_string_team(tmp_path: Path) -> None: + svc = _service(tmp_path) + wt = svc.get_worktree_path("guard-core", "backend", "be-dev-1", "a3c40fe7") + assert ( + wt + == tmp_path / "guard-core" / "backend" / "be-dev-1" / ".worktrees" / "a3c40fe7" + ) + + +def test_get_worktree_path_per_task_isolation(tmp_path: Path) -> None: + # Two tasks of the same agent get DISTINCT worktree dirs (the F123 point: + # each root its own checkout, never shared). + svc = _service(tmp_path) + a = svc.get_worktree_path("guard-core", Team.BACKEND, "be-dev-1", "a3c40fe7") + b = svc.get_worktree_path("guard-core", Team.BACKEND, "be-dev-1", "8e460893") + assert a != b + assert ( + a.parent + == b.parent + == tmp_path / "guard-core" / "backend" / "be-dev-1" / ".worktrees" + ) diff --git a/tests/unit/services/test_worktree_cleanup_on_complete.py b/tests/unit/services/test_worktree_cleanup_on_complete.py new file mode 100644 index 00000000..86589e8d --- /dev/null +++ b/tests/unit/services/test_worktree_cleanup_on_complete.py @@ -0,0 +1,169 @@ +"""Terminal worktree cleanup on complete/ceo_approve (F123 followup). + +Completed/merged tasks must not leak their per-task worktree on disk until the +whole agent is deleted. The two terminal→completed paths (cell-PM ``complete`` +after the leaf PR merges; CEO ``ceo_approve`` after root→master merges) remove +the assignee's worktree best-effort. Removal is terminal-only — a dev task +bounces ``needs_revision`` off the earlier review states and needs its worktree +back, so cleanup fires only at ``completed`` (post-merge, branch truly done). +No-op for branchless tasks (no worktree was ever cut). Best-effort: a removal +failure never blocks completion. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.models.base import TaskStatus +from roboco.services.task import TaskService + + +def _build_task(**overrides: object) -> MagicMock: + base: dict[str, object] = { + "id": uuid4(), + "status": TaskStatus.PENDING, + "branch_name": "feature/backend/abc12345", + "work_session_id": None, + "assigned_to": None, + } + base.update(overrides) + return MagicMock(**base) + + +def _bind(svc: TaskService, name: str, value: object) -> None: + object.__setattr__(svc, name, value) + + +def _slug_row(slug: str) -> MagicMock: + return MagicMock(scalar_one_or_none=MagicMock(return_value=slug)) + + +def _svc(execute: object) -> tuple[TaskService, MagicMock]: + # Build the session as a local MagicMock and preset `execute` on it before + # handing it to TaskService — assigning to `svc.session.execute` directly + # trips mypy's method-assign (session is typed as a real AsyncSession). + session = MagicMock() + session.execute = execute + session.flush = AsyncMock() + return TaskService(session), session + + +# --------------------------------------------------------------------------- +# complete (cell PM, awaiting_pm_review -> completed, after leaf PR merge) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_complete_removes_assignee_worktree_best_effort() -> None: + task = _build_task(status=TaskStatus.AWAITING_PM_REVIEW) + svc, _ = _svc(AsyncMock(return_value=_slug_row("roboco-api"))) + _bind(svc, "get", AsyncMock(return_value=task)) + _bind(svc, "_get_completing_agent_role", AsyncMock(return_value="cell_pm")) + _bind(svc, "_validate_completion_prerequisites", AsyncMock(return_value=[])) + _bind(svc, "_apply_complete_approval_chain", AsyncMock(return_value=None)) + _bind(svc, "_cancelled_force_allowed", MagicMock(return_value=True)) + _bind(svc, "_assert_pr_merged_for_complete", AsyncMock(return_value=True)) + _bind(svc, "_validate_and_set_status", MagicMock()) + _bind(svc, "_close_work_session_for_task", AsyncMock()) + _bind(svc, "_trigger_completion_hooks", AsyncMock()) + _bind(svc, "_unblock_dependents", AsyncMock()) + remove = AsyncMock() + _bind(svc, "_remove_task_worktree_best_effort", remove) + + result = await svc.complete(task.id) + + assert result is task + remove.assert_awaited_once_with(task, "roboco-api") + + +@pytest.mark.asyncio +async def test_complete_skips_worktree_cleanup_for_branchless_task() -> None: + # A branchless/umbrella task had no worktree cut — removal must be a no-op + # (and must not even probe the project slug). + task = _build_task(status=TaskStatus.AWAITING_PM_REVIEW, branch_name=None) + execute = AsyncMock() + svc, session = _svc(execute) + _bind(svc, "get", AsyncMock(return_value=task)) + _bind(svc, "_get_completing_agent_role", AsyncMock(return_value="cell_pm")) + _bind(svc, "_validate_completion_prerequisites", AsyncMock(return_value=[])) + _bind(svc, "_apply_complete_approval_chain", AsyncMock(return_value=None)) + _bind(svc, "_cancelled_force_allowed", MagicMock(return_value=True)) + _bind(svc, "_assert_pr_merged_for_complete", AsyncMock(return_value=True)) + _bind(svc, "_validate_and_set_status", MagicMock()) + _bind(svc, "_close_work_session_for_task", AsyncMock()) + _bind(svc, "_trigger_completion_hooks", AsyncMock()) + _bind(svc, "_unblock_dependents", AsyncMock()) + remove = AsyncMock() + _bind(svc, "_remove_task_worktree_best_effort", remove) + + result = await svc.complete(task.id) + + assert result is task + remove.assert_not_awaited() + session.execute.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_complete_not_blocked_by_worktree_removal_failure() -> None: + # Best-effort: a git/FS failure during cleanup must NOT fail the completion. + task = _build_task(status=TaskStatus.AWAITING_PM_REVIEW) + svc, _ = _svc(AsyncMock(return_value=_slug_row("roboco-api"))) + _bind(svc, "get", AsyncMock(return_value=task)) + _bind(svc, "_get_completing_agent_role", AsyncMock(return_value="cell_pm")) + _bind(svc, "_validate_completion_prerequisites", AsyncMock(return_value=[])) + _bind(svc, "_apply_complete_approval_chain", AsyncMock(return_value=None)) + _bind(svc, "_cancelled_force_allowed", MagicMock(return_value=True)) + _bind(svc, "_assert_pr_merged_for_complete", AsyncMock(return_value=True)) + _bind(svc, "_validate_and_set_status", MagicMock()) + _bind(svc, "_close_work_session_for_task", AsyncMock()) + _bind(svc, "_trigger_completion_hooks", AsyncMock()) + _bind(svc, "_unblock_dependents", AsyncMock()) + remove = AsyncMock(side_effect=RuntimeError("git worktree remove failed")) + _bind(svc, "_remove_task_worktree_best_effort", remove) + + result = await svc.complete(task.id) + + assert result is task # completion still succeeds + + +# --------------------------------------------------------------------------- +# ceo_approve (CEO, awaiting_ceo_approval -> completed, after root->master merge) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_ceo_approve_removes_assignee_worktree_best_effort() -> None: + task = _build_task(status=TaskStatus.AWAITING_CEO_APPROVAL, work_session_id=None) + svc, _ = _svc(AsyncMock(return_value=_slug_row("roboco-api"))) + _bind(svc, "get", AsyncMock(return_value=task)) + _bind(svc, "_validate_and_set_status", MagicMock()) + _bind(svc, "_extract_completion_learnings", AsyncMock()) + _bind(svc, "_unblock_dependents", AsyncMock()) + _bind(svc, "_emit_task_event", AsyncMock()) + remove = AsyncMock() + _bind(svc, "_remove_task_worktree_best_effort", remove) + + result = await svc.ceo_approve(task.id) + + assert result is task + remove.assert_awaited_once_with(task, "roboco-api") + + +@pytest.mark.asyncio +async def test_ceo_approve_skips_worktree_cleanup_for_branchless_task() -> None: + task = _build_task(status=TaskStatus.AWAITING_CEO_APPROVAL, branch_name=None) + svc, _ = _svc(AsyncMock()) + _bind(svc, "get", AsyncMock(return_value=task)) + _bind(svc, "_validate_and_set_status", MagicMock()) + _bind(svc, "_extract_completion_learnings", AsyncMock()) + _bind(svc, "_unblock_dependents", AsyncMock()) + _bind(svc, "_emit_task_event", AsyncMock()) + remove = AsyncMock() + _bind(svc, "_remove_task_worktree_best_effort", remove) + + result = await svc.ceo_approve(task.id) + + assert result is task + remove.assert_not_awaited() diff --git a/tests/unit/test_notification_dedup.py b/tests/unit/test_notification_dedup.py index fa2a71a8..3badef92 100644 --- a/tests/unit/test_notification_dedup.py +++ b/tests/unit/test_notification_dedup.py @@ -121,3 +121,97 @@ async def test_informational_knowledge_share_not_deduped() -> None: # Informational ⇒ NOT suppressed: a row was created + committed. db.add.assert_called_once() db.commit.assert_awaited_once() + + +# --------------------------------------------------------------------------- +# Bounded re-fire guard (loop-prone types) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_create_notification_suppresses_refire_when_guard_true() -> None: + """A re-fire (guard True) short-circuits before the DB dedup query AND + before any row is created/delivered — even though TASK_ASSIGNMENT is an + action-required type the DB dedup never fires for.""" + db = MagicMock() + db.scalar = AsyncMock() + db.add = MagicMock() + db.flush = AsyncMock() + db.commit = AsyncMock() + + svc = NotificationService() + cc: Any = svc + cc._resolve_recipients = AsyncMock(return_value=[uuid4()]) + params = CreateNotificationParams( + notification_type=NotificationType.TASK_ASSIGNMENT, + priority=NotificationPriority.NORMAL, + from_agent="from-1", + to_agents=["to-1"], + subject="s", + body="b", + related_task_id="t1", + ) + with ( + patch( + "roboco.services.notification.get_db_context", + return_value=_FakeDBCtx(db), + ), + patch( + "roboco.services.notification._resolve_agent_uuid", + AsyncMock(return_value=uuid4()), + ), + patch( + "roboco.services.notification.all_recipients_recently_notified", + AsyncMock(return_value=True), + ), + ): + await svc._create_notification(params) + + db.add.assert_not_called() + db.commit.assert_not_called() + db.scalar.assert_not_awaited() # returned before the DB dedup query + + +@pytest.mark.asyncio +async def test_create_notification_passes_through_when_guard_false() -> None: + """First fire (guard False) proceeds to row create + deliver.""" + db = MagicMock() + db.add = MagicMock(side_effect=lambda obj: setattr(obj, "id", uuid4())) + db.flush = AsyncMock() + db.commit = AsyncMock() + db.scalar = AsyncMock() # TASK_ASSIGNMENT is_ack_required=False → not awaited + + svc = NotificationService() + cc: Any = svc + cc._resolve_recipients = AsyncMock(return_value=[uuid4()]) + params = CreateNotificationParams( + notification_type=NotificationType.TASK_ASSIGNMENT, + priority=NotificationPriority.NORMAL, + from_agent="from-1", + to_agents=["to-1"], + subject="s", + body="b", + related_task_id="t1", + ) + with ( + patch( + "roboco.services.notification.get_db_context", + return_value=_FakeDBCtx(db), + ), + patch( + "roboco.services.notification._resolve_agent_uuid", + AsyncMock(return_value=uuid4()), + ), + patch( + "roboco.services.notification.all_recipients_recently_notified", + AsyncMock(return_value=False), + ), + patch( + "roboco.services.notification_delivery.get_notification_delivery_service", + lambda _db: MagicMock(deliver=AsyncMock(return_value=None)), + ), + ): + await svc._create_notification(params) + + db.add.assert_called_once() + db.commit.assert_awaited_once() diff --git a/tests/unit/test_notification_dedup_refire.py b/tests/unit/test_notification_dedup_refire.py new file mode 100644 index 00000000..21248f6e --- /dev/null +++ b/tests/unit/test_notification_dedup_refire.py @@ -0,0 +1,213 @@ +"""Bounded re-fire guard for loop-prone notification types. + +TASK_ASSIGNMENT / REVIEW_REQUEST / DOCUMENTATION_REQUEST / BROADCAST can be +re-fired in a loop (a PM re-notifying the same recipient about the same task +every tick while it sits in a state), flooding inboxes. A short Redis SET-NX +window per (type, sender, recipient, task) suppresses the re-fire. Fail-open: +Redis unavailable → never suppresses. KNOWLEDGE_SHARE / MENTION / A2A_REQUEST +always pass through (one-shot by nature, no dedup key). +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import uuid4 + +import pytest +from roboco.models import NotificationType +from roboco.services.notification_dedup import all_recipients_recently_notified + +_FAKE_URL = "redis://localhost:6379/0" +_DEDUP_TTL = 60 # mirrors _DEDUP_TTL_SECONDS in the helper +_TWO_RECIPIENTS = 2 + + +def _conn(set_returns: list[object]) -> MagicMock: + """A fake redis conn whose `.set` returns successive values then None.""" + c = MagicMock() + c.set = AsyncMock(side_effect=[*set_returns, None]) + c.aclose = AsyncMock() + return c + + +@pytest.mark.asyncio +async def test_first_fire_not_suppressed() -> None: + # First fire for a single recipient: SET NX acquires (True) → not a re-fire. + conn = _conn([True]) + with ( + patch("roboco.services.notification_dedup.settings") as settings, + patch("roboco.services.notification_dedup.redis") as redis_mod, + ): + settings.redis_url = _FAKE_URL + redis_mod.from_url.return_value = conn + a = uuid4() + suppressed = await all_recipients_recently_notified( + ntype=NotificationType.TASK_ASSIGNMENT, + from_agent=uuid4(), + recipients=[a], + related_task_id=uuid4(), + ) + assert suppressed is False + conn.set.assert_awaited_once() + assert conn.set.call_args.kwargs.get("nx") is True + assert conn.set.call_args.kwargs.get("ex") == _DEDUP_TTL + + +@pytest.mark.asyncio +async def test_all_recipients_dup_suppresses() -> None: + # Two recipients, both already held (SET NX returns None for each) → re-fire. + conn = _conn([None, None]) + with ( + patch("roboco.services.notification_dedup.settings") as settings, + patch("roboco.services.notification_dedup.redis") as redis_mod, + ): + settings.redis_url = _FAKE_URL + redis_mod.from_url.return_value = conn + suppressed = await all_recipients_recently_notified( + ntype=NotificationType.REVIEW_REQUEST, + from_agent=uuid4(), + recipients=[uuid4(), uuid4()], + related_task_id=uuid4(), + ) + assert suppressed is True + assert conn.set.await_count == _TWO_RECIPIENTS + + +@pytest.mark.asyncio +async def test_mixed_recipients_not_suppressed() -> None: + # One fresh (acquired) + one dup → persist (not suppress). The fresh one is + # acquired (marked) so the next fire converges toward full suppression. + conn = _conn([True, None]) + with ( + patch("roboco.services.notification_dedup.settings") as settings, + patch("roboco.services.notification_dedup.redis") as redis_mod, + ): + settings.redis_url = _FAKE_URL + redis_mod.from_url.return_value = conn + suppressed = await all_recipients_recently_notified( + ntype=NotificationType.DOCUMENTATION_REQUEST, + from_agent=uuid4(), + recipients=[uuid4(), uuid4()], + related_task_id=uuid4(), + ) + assert suppressed is False + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "ntype", + [ + NotificationType.KNOWLEDGE_SHARE, + NotificationType.MENTION, + NotificationType.A2A_REQUEST, + ], +) +async def test_excluded_types_never_suppressed(ntype: NotificationType) -> None: + # One-shot types bypass the guard entirely — even if Redis would say dup. + conn = _conn([None]) + with ( + patch("roboco.services.notification_dedup.settings") as settings, + patch("roboco.services.notification_dedup.redis") as redis_mod, + ): + settings.redis_url = _FAKE_URL + redis_mod.from_url.return_value = conn + suppressed = await all_recipients_recently_notified( + ntype=ntype, + from_agent=uuid4(), + recipients=[uuid4()], + related_task_id=uuid4(), + ) + assert suppressed is False + conn.set.assert_not_awaited() # guard short-circuited before touching Redis + + +@pytest.mark.asyncio +async def test_redis_unavailable_fail_open() -> None: + # Redis down / from_url raising → never suppress (a notification is never + # dropped because of the dedup infra). + with ( + patch("roboco.services.notification_dedup.settings") as settings, + patch("roboco.services.notification_dedup.redis") as redis_mod, + ): + settings.redis_url = _FAKE_URL + redis_mod.from_url.side_effect = RuntimeError("redis down") + suppressed = await all_recipients_recently_notified( + ntype=NotificationType.BROADCAST, + from_agent=uuid4(), + recipients=[uuid4()], + related_task_id=None, + ) + assert suppressed is False + + +@pytest.mark.asyncio +async def test_empty_recipients_or_no_sender_short_circuits() -> None: + # Nothing to dedup against → not suppressed, no Redis call. + with ( + patch("roboco.services.notification_dedup.settings") as settings, + patch("roboco.services.notification_dedup.redis") as redis_mod, + ): + settings.redis_url = _FAKE_URL + redis_mod.from_url.return_value = _conn([]) + assert ( + await all_recipients_recently_notified( + ntype=NotificationType.TASK_ASSIGNMENT, + from_agent=uuid4(), + recipients=[], + related_task_id=uuid4(), + ) + is False + ) + assert ( + await all_recipients_recently_notified( + ntype=NotificationType.TASK_ASSIGNMENT, + from_agent=None, + recipients=[uuid4()], + related_task_id=uuid4(), + ) + is False + ) + redis_mod.from_url.assert_not_called() + + +@pytest.mark.asyncio +async def test_key_carries_type_sender_recipient_and_task() -> None: + # The dedup identity is (type, sender, recipient, task) — rewording or a + # different subject must NOT defeat the guard, and 'none' stands in for a + # taskless broadcast so two broadcasts about nothing still dedup. + conn = _conn([True]) + sender = uuid4() + recip = uuid4() + task = uuid4() + with ( + patch("roboco.services.notification_dedup.settings") as settings, + patch("roboco.services.notification_dedup.redis") as redis_mod, + ): + settings.redis_url = _FAKE_URL + redis_mod.from_url.return_value = conn + await all_recipients_recently_notified( + ntype=NotificationType.TASK_ASSIGNMENT, + from_agent=sender, + recipients=[recip], + related_task_id=task, + ) + key = conn.set.call_args.args[0] + assert key == f"roboco:notif_dedup:task_assignment:{sender}:{recip}:{task}" + + conn2 = _conn([True]) + with ( + patch("roboco.services.notification_dedup.settings") as settings, + patch("roboco.services.notification_dedup.redis") as redis_mod, + ): + settings.redis_url = _FAKE_URL + redis_mod.from_url.return_value = conn2 + await all_recipients_recently_notified( + ntype=NotificationType.BROADCAST, + from_agent=sender, + recipients=[recip], + related_task_id=None, + ) + assert ( + conn2.set.call_args.args[0] + == f"roboco:notif_dedup:broadcast:{sender}:{recip}:none" + ) diff --git a/tests/unit/test_notification_delivery_refire.py b/tests/unit/test_notification_delivery_refire.py new file mode 100644 index 00000000..9eb9f75f --- /dev/null +++ b/tests/unit/test_notification_delivery_refire.py @@ -0,0 +1,78 @@ +"""NotificationDeliveryService._persist_and_deliver re-fire guard. + +Path 2 bypasses the DB dedup in NotificationService._create_notification, so +the same 60s Redis SET-NX guard gates it. Suppress (skip add/deliver) when the +guard says every recipient was just notified; pass through otherwise. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch +from uuid import uuid4 + +import pytest +from roboco.models import NotificationPriority, NotificationType +from roboco.services.notification_delivery import NotificationDeliveryService + + +def _notification( + ntype: NotificationType = NotificationType.TASK_ASSIGNMENT, +) -> MagicMock: + n = MagicMock() + n.id = uuid4() + n.type = ntype + n.from_agent = uuid4() + n.to_agents = [uuid4(), uuid4()] + n.related_task_id = uuid4() + n.priority = NotificationPriority.NORMAL + n.subject = "s" + n.body = "b" + n.requires_ack = True + return n + + +def _svc(session: MagicMock) -> Any: + """Build the service with ``deliver`` stubbed via an Any-typed alias so the + reassignment stays type-clean (no method-assign suppression).""" + svc = NotificationDeliveryService(session) + cc: Any = svc + cc.deliver = AsyncMock() + return svc + + +@pytest.mark.asyncio +async def test_persist_and_deliver_suppresses_when_guard_true() -> None: + session = MagicMock() + session.add = MagicMock() + session.flush = AsyncMock() + svc = _svc(session) + + with patch( + "roboco.services.notification_delivery.all_recipients_recently_notified", + AsyncMock(return_value=True), + ): + await svc._persist_and_deliver(_notification()) + + session.add.assert_not_called() + session.flush.assert_not_awaited() + svc.deliver.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_persist_and_deliver_passes_through_when_guard_false() -> None: + session = MagicMock() + session.add = MagicMock() + session.flush = AsyncMock() + svc = _svc(session) + + notif = _notification() + with patch( + "roboco.services.notification_delivery.all_recipients_recently_notified", + AsyncMock(return_value=False), + ): + await svc._persist_and_deliver(notif) + + session.add.assert_called_once_with(notif) + session.flush.assert_awaited_once() + svc.deliver.assert_awaited_once()