From 3aff6e04c04aec1160bce8fffaa452b728732756 Mon Sep 17 00:00:00 2001 From: Renzo F <45401804+rennf93@users.noreply.github.com> Date: Mon, 29 Jun 2026 11:32:34 +0200 Subject: [PATCH] Chore: Close gaps (#285) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Updated uv.lock * Bunch of fixes we need to verify first.. * feat(megatask): per-cell project map root-subtasks (multi-project, multi-cell) A MegaTask root-subtask can now target an ad-hoc per-cell project map — a third targeting shape that mirrors the existing product fan-out root. In RoboCo a project is per-cell (ProjectTable.assigned_cell); a monorepo is N per-cell projects sharing one git_url. So 'multi-cell' IS 'multi-project', and a task may mix per-cell projects across products or include OSS-library projects not in any product. Storage: migration 052 adds task_cell_projects (mirrors product_projects; unique per (task, team)). TaskTable gains a cascade-delete cell_projects relationship; TaskCreateRequest / TaskCreate / Task response carry the map. Policy: batch.is_branchless_coordination + is_valid_batch_shape gain a has_cell_projects param — a root-subtask targets exactly one of project / product / cell-map; the umbrella still targets none. TaskService passes has_cell_projects at every predicate call site and persists the rows in create(). _ensure_branch_for_task cuts feature/main_pm/{root} per distinct project in the map (via _distinct_projects_for_task); _require_target_or_umbrella and _validate_batch_membership accept the map shape. Fan-out: every distinct_project_ids site (task.py branch creation, routes _project_for_complete + _resolve_project_for_merge, orchestrator _ambient_projects_for_task, pr_review._project_slug_for, git._project_for_task) generalizes to first-distinct-project-of-map-or-product. Choreographer _resolve_subtask_project resolves a delegated subtask's cell from the parent's cell map. The product-scoped _slugs_for_product intake helper is unchanged. Intake: prompter._draft_cell_map extracts the per-cell map from the_work[]. _validate_batch_scope counts distinct projects across all drafts' cells (>=2 min stays; one 2-cell draft satisfies it). create_task_from_draft persists cell_projects for >=2-cell drafts (project_id/product_id None), collapses a 1-cell map to the single-project shape, and leaves single-cell top-level project_id drafts unchanged. _resolve_owning_team routes a multi-cell map to Main PM (coordination root, like a product root — a cell PM can't delegate cross-cell). propose_draft/propose_batch tool descriptions declare the per-cell project_id (both Claude SDK + grok runtimes). The umbrella stays branchless / pure-coordination / submit_root-rejected; the CEO-escalation pr_number gate is not widened (the map root is is_umbrella=False, mirroring a product root, so submit_root supplies it). Single-cell root-subtasks and everything below them are byte-for-byte unchanged. Un-run MegaTask waves (multi-cell drafts) become runnable. * [feature] Panel per-cell project picker + pnpm format infra MegaTask root-subtasks can fan out across cells (be+fe, fe+uxui). Since a RoboCo project is per-cell (ProjectTable.assigned_cell), a monorepo is N per-cell projects sharing one git_url — so multi-cell IS multi-project. The batch-review card now shows one project Select per the_work entry, scoped to that cell's repos, instead of one Select bound to a single top-level project_id. confirmBatch validates each cell's project is in scope and the batch still spans >=2 distinct projects. - prompter.ts: CellWork gains optional project_id (the per-cell picker seam). - batch-review-card.tsx: per-cell Selects (one per the_work entry), scoped to the cell's projects; legacy single-cell drafts keep the one-Select path. - use-prompter.ts: updateBatchDraftProject edits per-cell (entryIndex); confirmBatch validates every cell; batchFromEvent parses per-cell map. Also adds the missing pnpm format infrastructure (the panel had no formatter at all): prettier devDep + .prettierrc.json (default-style config: 80-col, double-quote, semi, trailing-comma-all) + .prettierignore, plus format / format:check scripts. Only the 3 changed files above were reformatted; the ~222 pre-existing non-compliant files are left untouched (a wholesale reformat is a separate explicit decision, not bundled into this feature). * [fix] MegaTask verification: migration 052 enum + async cell-map read Two real bugs surfaced running the full gate against a containerized Postgres (and the orchestrator boot log): 1. Migration 052 crashed a real orchestrator boot with 'type "team" already exists'. The generic sa.Enum(create_type=False) does NOT set the postgres enum's create_type attribute, so op.create_table (checkfirst=False) emitted a redundant CREATE TYPE against the pre-existing team enum. Switched to postgresql.ENUM(create_type=False) — the postgres- native enum whose create_type _check_for_name_in_memos actually reads, so the CREATE TYPE is suppressed. Verified: 051->052 upgrade against a DB where the team enum pre-existed (the exact path that crashed) now succeeds; downgrade 052->051 drops the table and preserves the shared enum; fresh upgrade head clean. (Migration 016 has the same latent sa.Enum pattern but never re-runs in prod, so it's noted, not touched here.) 2. _ensure_branch_for_task read task.cell_projects (lazy=selectin to-many) directly, tripping MissingGreenlet on a freshly-created/unqueried task — which then poisoned the async session (PendingRollbackError). Replaced with _task_has_cell_map: peeks InstanceState.unloaded (no IO) and reads the already-loaded map, falling back to an awaited count query only when the relationship is genuinely unloaded. Non-ORM stubs route to the plain attribute. Fixes 2 integration tests; the 6 cell-map unit tests still pass. Also: typed the self stub as Any in test_choreographer_subtask_project (mypy tests/ wants Choreographer, not SimpleNamespace) — the codebase idiom. Gate: ruff format/check clean; mypy roboco/ + tests/ clean; full pytest 10371 passed / 388 skipped against containerized pgvector:pg16; vulture clean. Pre-existing xenon C-rank on reassign (from prior commit 19a474d3, not this feature) still blocks make quality — surfaced separately. * [refactor] Extract reassign board-advisory diversion helper (C→B complexity) `reassign` in roboco/services/task.py hit xenon absolute complexity 11 (a C-rank block), failing `make quality`'s --max-absolute B gate. The C-rank originated in 19a474d3 (pre-existing, not this feature branch's work). Extract the board/advisory → cell-task diversion into `_maybe_divert_board_advisory_reassign` (complexity 4, A). reassign drops to 9 (B); behavior is byte-for-byte preserved — the helper runs the same guard + pool diversion + log, returning the diverted task or None so the caller falls through to the normal handoff. Whole-repo xenon exits 0; the 159 reassign / board-guard tests pass. Unblocks `make quality` on feature/metrics-granularity. * [fix] migration 016: postgresql.ENUM(create_type=False) for reused team enum 016_add_products_and_task_product_id used `sa.Enum(..., create_type=False)` for the reused Postgres "team" enum — the same latent defect that crashed 052 on a real orchestrator boot. On the generic `sa.Enum` the `create_type` kwarg is silently dropped, so `_check_for_name_in_memos` never sees it and `op.create_table` (checkfirst=False) emits a redundant `CREATE TYPE team` that fails with "type 'team' already exists" against a DB where the enum pre-exists. Switch to the postgres-native `postgresql.ENUM(..., create_type=False)` — its `create_type` is a real attribute the guard reads, so the CREATE TYPE is suppressed (and DROP TYPE on downgrade too). The member list is inert under create_type=False (it never creates/alters the type), so it stays at 016's original six, reflecting the enum as it stood then, not the later-widened set. This never crashed in prod because 016 is never re-run (alembic_version is past it), but it's the same defect class. Verified on the real boot path: upgrade to 015 in process A (team enum created by 001), then `upgrade head` in a fresh process B — 016 applied clean, no DuplicateObjectError; downgrade 016->015 clean, shared team enum preserved. See project_migration_enum_create_type_gotcha. * [chore] panel: prettier reformat across the codebase Apply `pnpm format` (prettier 3.8.5, 80-col / double-quote / semi / trailing-comma-all) to the 223 pre-existing panel files that predated the prettier infra added in cb5365a4. Pure formatting — no semantic changes: multi-line arrays/objects collapsed where they fit, trailing newlines added (.prettierrc.json), import grouping unchanged. Verified: `pnpm format:check` clean, `pnpm lint` clean, `pnpm typecheck` clean, `pnpm test` 113/113 pass (7 files). * Bunch of runtime fixes for MegaTask and other issues * Fix different project same PR number collision problem Fix (two layers): 1. Root cause — pr_merge and rebase_pr_for_task now take a required project_id and scope the lookup where(pr_number == X AND project_id == Y). Required so no caller can forget — the bug class can't recur. All 4 call sites updated (choreographer cell_pm_complete, the rebase-retry, the superseded close_pull_request now passes project_id, and _verb_runner._do_pr_merge). 2. Crash guard — _finalize_cell_complete None-checks the complete() return and returns a clean invalid_state envelope (with a remediate hint) instead of dereffing None → 500 → respawn loop. * Fix: Make main_pm + task_type=code impossible * Fix Main PM needs revision can't re delegate * [chore] Bump local LLM glm-5→glm-5.2 + swap Ollama fleet defaults off minimax - llm_catalog: OLLAMA_DEFAULT_MODEL minimax-m3:cloud → kimi-k2.7-code:cloud; role defaults kimi-k2.6→kimi-k2.7-code, developer minimax→kimi, product_owner/ ceo kimi→glm-5.2, documenter glm→kimi; GLM 5.1→5.2 comment fix. - config + .env.example + docker-compose{.yml,.yaml,.registry.yml} + docs + memory_distiller + optimal_brain: glm-5:cloud → glm-5.2:cloud. - panel ai-routing-card: typed SelfHostedModel/boolean annotations; drop the stale "Minimax M3 default" string (default is now catalog-driven). - tests: glm-5:cloud → glm-5.2:cloud in pricing + rate-limit-retry fixtures. * [fix] submit_root: hard unchanged-PR gate stops the pr_fail re-submit loop The 2026-06-27 infinite pr_fail loop: a Main-PM root (PR #139) was pr_fail'd, routed to needs_revision, and re-submitted byte-identical → awaiting_pr_review → pr_fail again, forever. The prior hint/a2a steer was ignored by the weak coordinator model — hints don't stop a model that won't read them. A HARD gate refuses the re-submit when the assembled root PR's head SHA is unchanged since the last pr_fail (no new cell work → identical diff); a different SHA ⇒ the branch advanced ⇒ allow. Every ambiguous case fails open (no prior fail, no recorded SHA, no pr_number, unresolvable slug, git error, closed PR) — only the exact-unchanged case is hard-blocked. - content/models: PrReviewContent.head_sha (optional; JSON col → no migration). - git: get_pr_head_sha (GitHub pulls API; None on any failure → fail-open). - pr_gate: pr_fail captures head_sha into the verdict record; pr_pass does not. - _impl: submit_root runs _submit_root_unchanged_pr_guard after _submit_up_guard; _current_root_pr_head_sha resolves slug + current SHA (fail-open). - pr_review: extract module-level resolve_task_project_slug, shared by the mixin and the gate helper (_LegacyChoreographer reaches it via cast to the ChoreographerHelpers typed view — it doesn't inherit the helpers mixin). - tests: test_submit_root_unchanged_pr_guard (11 — refuse/allow/6 fail-open/3 capture-side, mypy-clean via cc:Any spy idiom, zero type:ignore) + test_pr_gate_notifies_pm capture-path stub. * [chore] mypy tests/: clear all 15 pre-existing type errors so make quality can go green The branch tip had 15 mypy tests/ errors in files this bundle did not author, which blocked CI's make quality mypy step (mypy roboco/ tests/) regardless of the bundle's own commits. Pre-existing is still existing — fix every one: - test_schemas_v1_flow.py (8): the StrList coercion tests intentionally pass SDK-nested list-of-strings input ([[['...']]], {'item':{'$text':'...'}}, int, dict). Annotate those literals as list[Any] locals so mypy accepts the coerce-able shape; the StrList BeforeValidator still flattens to list[str] at runtime. No type:ignore. - test_pr_gate_records_verdict.py (3): notes_structured is dict|None; narrow with 'assert t.notes_structured is not None' before indexing (the existing pattern at line 90). - test_pr_review_hand_format_guard.py (1 site, 2 errors): the _verb_runner() spy assertion — use the cc: Any = c alias idiom so assert_not_awaited resolves; drops the now-unused type:ignore[union-attr]. - test_pr_gate_notifies_pm.py (1): drop the unused type:ignore[method-assign] on the a2a.send reassignment. - test_content_models.py (1): narrow coerced with isinstance(coerced, PrReviewContent) before reading .issues (the base _Content lacks the field). Gates: rm -rf .mypy_cache && mypy roboco/ tests/ = Success (855 files); ruff check + format clean; 5 affected suites = 40 passed. * [fix] fail_qa routes needs_revision back to the dev, never the pool A dev task in needs_revision must go back to the developer, never the pool. The pool path let a cell PM re-claim the revision (PMs can claim needs_revision) — the live 2026-06-27 'needs revision on a dev task sent to the cell PM' bug. fail_qa's original_developer marker is the fast path, but it is unreliable in practice (live observation: never persisted), so the unassign else-branch was the load-bearing path and it dropped the task into the pool. Add a work-session fallback (_resolve_revision_dev) that resolves the developer who actually worked the task — the most recent work session whose agent is a developer, the QA's own session excluded — and reassigns to that dev instead of unassigning. Only unassign when no developer ever touched the task. Self-heals the marker so a subsequent re-fail takes the fast path and the QA-review index attributes the work correctly. * [feature] delegate carries dev-task collision surface (sequencing S1) The cell/main PM's delegate verb now carries the dev-task collision surface (intends_to_touch / adds_migration / touches_shared) and an explicit depends_on override through DelegateRequest -> DelegateInputs -> _create_subtask_from_inputs -> create_subtask, and create_subtask forwards sequence / dependency_ids / batch_id / surfaces into the prepared TaskCreateRequest instead of dropping them (the base create already persists them at task.py:878-884). This is the plumbing for the multi-level sequencing model edge kind 3 (dev-task collision DAG). Previously a dev task delegated with a collision surface or an explicit dependency lost it before persistence — dependency_ids was always [], so the only dev-task ordering was the weak assignee-keyed spawn barrier (the live 2026-06-27 out-of-order break: 40842957 started before 9b3682b8's PR merged). Phase S2 runs SequencingService over the surfaced siblings and wires the DAG via add_dependency. * [feature] wire dev-task collision DAG at cell-PM delegation (sequencing S2) Pure dev_task_collision_edges in sequencing.py turns a parent's surfaced siblings into (depends_on_id, task_id) pairs via SequencingService. TaskService. wire_sibling_collision_dag wires them through add_dependency (idempotent). The choreographer calls it after each dev-task delegate so the sibling collision DAG is built incrementally as the cell PM decomposes — file-overlap serializes, migration chains, shared-last; stable (priority, sequence) ordering keeps edges from flipping into reverse cycles on re-runs. * [feature] wire cell-task wave chain + by-osmosis edge (sequencing S3) Kind 2 (cell-task wave chain): a new cell-task under root-subtask UT_n depends on every cell-task under every root-subtask in UT_n.dependency_ids (the kind-1 wave-chain edges), so its branch carries the previous wave's merged cell work. Re-derived from the root-subtask's deps, not the cell-task's own dependency_ids (which also carry UX/product-fanout edges the by-osmosis edge must not pick up). A root may fan to several cell-tasks (different cells), so the previous wave's cell-task is a SET. Kind 4 (by-osmosis): the first dev task (sequence 0) under a cell-task depends on each predecessor cell-task's tail (max-sequence) dev task, so the new wave's first branch carries the previous wave's fully-merged tail. Subsequent dev tasks inherit the tail via kind 3 or the merged base. Both wired from _create_subtask_from_inputs, dispatched on parent.team (MAIN_PM -> kind 2; cell team -> kind 4). Pure helpers (cell_task_wave_chain_depends_on, by_osmosis_tail_dev_tasks) unit-tested in test_sequencing.py; TaskService methods integration-tested. Idempotent + best-effort throughout (add_dependency dedupes; missing predecessors are no-ops). Also fixes a latent mypy-tests gap (estimated_complexity required on direct TaskCreateRequest calls in the S2 tests). * [feature] sync_branch dev verb — gate-level branch rebase (Phase B1) Raw shell git is denied to agents (Bash(git:*) base deny), so a developer whose branch fell behind its base had no gate-level rebase — only the CEO/PM-only /rebase HTTP route. sync_branch is the dev verb that wraps the rebase through the gate (traced + evidenced), so the 'everything goes through the gates' invariant holds. - lifecycle: IntentSpec sync_branch (dev-only, ownership-gated, composes=(), git-only — no DB transition); _next_hint_synced helper. - GitService.sync_task_branch: rebase task.branch_name onto its resolved base via rebase_onto_base (fetch + rebase + force-with-lease push). - Choreographer.sync_branch + _sync_branch_preflight_rejection: not_found / unknown-role / spec-gate / no-branch / protected-base guards, then the git op; conflicts abort (no force-push) and steer to resolve-by-hand; git failure steers to i_am_blocked. - HTTP route /api/v1/flow/developer/sync_branch + SyncBranchRequest schema. - MCP tool sync_branch(task_id) + _TOOLS registration (manifest auto-propagates via intents_for_role(Role.DEVELOPER)). Tests: intent spec (5), choreographer handler (8: happy/conflicts/not_found/ not_authorized/no-branch/protected-base/git-failure/audit), route (1), MCP (1). ruff + mypy roboco/ tests/ clean; unit suite green (DB-fixture errors env-only). * [feature] i_am_done behind-base submit gate (Phase B2) A sibling's PR merging into the parent branch while a dev worked leaves the dev's branch behind its base — the assembled PR then can't merge cleanly and the sibling's changes go missing (the 2026-06-27 out-of-order dev-task break). The behind-base gate refuses i_am_done in that state and steers the dev to sync_branch (the Phase B1 gate-level rebase verb). - GitService.is_behind_base: rev-list --left-right --count across origin/{base}...origin/{head} → (behind, ahead); fetch-first so origin reflects the pushed head. Raises on git failure (consistent with rebase_onto_base); malformed stdout degrades to (0,0). - Choreographer._behind_base_gate: wired into _i_am_done_gate after _ensure_branch_pushed. behind>0 → invalid_state remediate→sync_branch. Fail-open on git/base-resolution error (flaky fetch can't strand a task at the submit gate — the merge layer has its own behind checks). Skipped for branchless roots and protected bases (master/main/-prefixed). Tests: gate (6: refuse+steer/up-to-date/branchless/protected/fail-open-base/ fail-open-git), is_behind_base (6: parse/up-to-date/malformed/argv-form/ requires-branch/missing-project). ruff + mypy roboco/ tests/ clean; unit green. * [docs] sync_branch prompt + behind-base guidance (Phase B3) Update every behind-base/rebase guidance surface to reflect the B1 sync_branch dev verb + B2 i_am_done behind-base gate: devs now self-rebase through the gate instead of escalating a plain behind-base condition; PMs still escalate cell/root integration branches (they have no rebase verb). - developer.md: sync_branch in the verb table; 'When your branch is behind its base' rewritten — call sync_branch, do NOT i_am_blocked a plain behind-base; conflicts → resolve by hand, commit, sync_branch again. - cell_pm.md: delegate signature gains intends_to_touch/adds_migration/ touches_shared/depends_on + a 'Collision surface' section (fill it on every code subtask so sibling dev tasks that touch the same files sequence into a conflict-free order — the 2026-06-27 out-of-order break fix); behind-base section steers devs to sync_branch, PMs escalate only the integration branch. - main_pm.md: behind-base section — dev leaf = dev's sync_branch; cell/root integration branch = escalate_up. - RAG git-errors.md / blocked-tools.md: devs sync_branch, PMs escalate. - docs/troubleshooting/common-issues.md: leaf self-rebases; integration branch still escalates to operator. - CLAUDE.md verb surface: developer gains sync_branch. - agents/prompts/_generated/*: regenerated via scripts/regenerate_verb_tables.py — adds sync_branch to the dev table AND catches the generated tables up to the S1/S2 delegate sequencing params + meltdown-fix note top-level params (the derived files had drifted stale vs the already-committed schemas). Docs/prompts only — no code. ruff + mypy roboco/ tests/ clean. * [chore] orchestrator: refuse to spawn human-only roles (CEO/prompter/secretary) A live 2026-06-27 incident saw a CEO agent container spawned. Root cause: _dispatch_a2a_work iterates every A2A/notification target and spawns it with no human-role filter, and _is_agent_active('ceo') is always false (the CEO is never a container), so the 'skip if active' check could never protect the CEO. Any CEO-addressed notification (board handoff, escalation) launched a CEO container — the system acting as the human CEO: a trust violation. The CEO is the human operator; intake (prompter) and secretary are human-driven chats launched through their own dedicated guarded paths (_spawn_intake_container / _spawn_secretary_container), never spawn_agent. Fix: a single chokepoint guard at the top of spawn_agent refuses Role.CEO / PROMPTER / SECRETARY (raises AgentReadinessError + logs). This structurally covers every dispatcher present and future, since they all go through spawn_agent. Plus a defense-in-depth skip in _dispatch_a2a_work so a human-role target never even calls in (avoids error-log spam; the notification stays for the human to read in the panel). Safe: the dedicated human-spawn paths do not route through spawn_agent. Regression tests: spawn_agent refuses ceo/intake-1/secretary-1, does NOT refuse a real agent; _dispatch_a2a_work skips CEO/intake/secretary targets and still spawns real-agent + mixed-target cases. * [chore] orchestrator: skip human-only assignees in claimed/pm-review dispatchers Defense-in-depth for the spawn_agent human-role chokepoint (d31d6719). The chokepoint structurally guarantees no CEO/prompter/secretary container can ever spawn — every dispatcher goes through spawn_agent. But two dispatchers resolve an arbitrary assigned_to and spawn it with only a None/unknown-role filter, so a human-assigned task would reach the chokepoint and RAISE: caught by the per-dispatcher try/except, but it aborts that dispatcher's whole tick (stalling other respawns behind the mis-assigned task) and error-logs every cycle. The other dispatchers are already safe by whitelist/hardcoded slug (blocker_resolver_slug returns None for non-PM/non-BOARD; escalation/approval use whitelists; marketing and audit hardcode their non-human slug). - _claimed_task_needs_agent: return None for a CEO/prompter/secretary assignee — no container to respawn, and do NOT release a human-owned task to pending (that would re-route it to a PM). Leave it for the human. - _dispatch_pm_review_work (assigned branch): skip a human-only assignee so a CEO-assigned awaiting_pm_review task neither spawns nor aborts the dispatcher's tick. Audited all target-iterating dispatchers; only these two lacked a filter. Regression tests cover both skips. * [F002] retype board-routed MegaTask root-subtasks code->planning on activation _activate_batch_root_subtasks flipped a held root-subtask to team=MAIN_PM but left task_type=code (intake only coerces main_pm-team drafts, so a board-routed code root reached activation still code-typed). The main_pm+code combo re-introduces the 2026-06-27 meltdown. Mirror approve_and_start's own retype via main_pm_cannot_own_code so the activated child is a planning-typed coordination root. TDD: RED test_activate_batch_root_subtasks_retypes_code_to_planning watched fail (task_type stayed CODE), then GREEN after the retype. ruff+mypy clean; 125 batch/umbrella/approve tests green, no regressions. * [F003,F004,F014] enforce HMAC agent-token gate on do routes + WebSocket streams F003/F014: /api/v1/do/* only required X-Agent-ID (UUID) — no token check, unlike the flow routers' role guards. A forged X-Agent-ID passed. Added require_any_authenticated_agent (token-only; do router serves all roles) and applied it as a router-level dependency. Binds X-Agent-ID to a verified HMAC token when ROBOCO_AGENT_AUTH_REQUIRED=true; rejects a forged token even in dev mode. F004: /ws/* per-agent streams (channels/agents/sessions/notifications) never read the nginx-injected X-Agent-Token, so in strict mode an agent on the Docker network could subscribe to another agent's notifications with no auth. Added _require_panel_token verifying the CEO panel token against the CEO identity; wired into all four per-agent streams (system stream stays operator-only per its docstring). Same strict/dev contract. TDD: RED tests watched fail (no gate -> 200/accept), then GREEN. ruff+mypy clean; 399 api/mcp + 29 WS tests green, no regressions. * [F005,F006] grok auth: directory mount + atomic-write fallback F005: the single-file bind mount of auth.json pinned the inode, so the orchestrator's atomic refresh (tmp+rename within ~/.grok) never reached a running grok container — a long-lived container hung at the login prompt when the original ~6h token expired. Mount the host ~/.grok DIRECTORY (ro) at /home/agent/.grok-auth-ro; the entrypoint symlinks ~/.grok/auth.json at that RO mount so grok + the --check backstop read the live credential (the directory mount sees the host-side rename) while grok's writable state (config.toml, sessions/) stays in the image's ~/.grok. F006: a rotated refresh_token is single-use — xAI invalidates the old one the instant it issues the new one. If the atomic write failed after the rotation, the file kept the now-dead old refresh_token and the credential was permanently lost on the next refresh. _atomic_write now falls back to a direct write when tmp+replace fails, so the rotated token always lands on disk (losing the write is catastrophic; losing atomicity is not). TDD: RED tests watched fail, then GREEN. ruff+mypy clean; 32 grok tests green, no regressions. * [F016,F017] choreographer: surface invalid_state instead of None.status 500 on submit_root / i_am_blocked Both verbs compose a single atomic action whose None return (the verb's own result) flowed out of run_intent and was dereferenced as t.status, HTTP 500-ing with no actionable rejection: - F016 submit_root: submit_for_review returns None when the root->master PR was already opened / the task raced out of in_progress. Post-runner None-guard extracted into _submit_root_finalize -> invalid_state (re-fetch; if awaiting_pr_review the PR is open, wait for reviewer; else re-delegate fixes and retry) instead of None.status. - F017 i_am_blocked: escalate returns None in four cases (no task, no agent, no resolvable escalation-target slug, no target agent row) e.g. a developer whose role has no PM above it. _run_i_am_blocked_intent now guards updated is None -> (t, invalid_state rejection) with remediation (re-fetch + escalate to CEO directly / retry) instead of the caller deref'ing None.status -> 500 + respawn-loop. TDD red->green; ruff + mypy clean; gateway suite green (58 passed). * [F007] choreographer: cell-level unchanged-PR re-submit loop-stopper for submit_up The root loop-stopper (F016) was root-only; a weak cell PM could re-submit the unchanged cell->root PR after a pr_fail and loop awaiting_pr_review -> pr_fail forever (the cell analogue of the 2026-06-27 root loop). pr_fail stamps the assembled PR's head SHA into notes_structured.pr_review .head_sha for cell AND root gate tasks alike (the capture is gate-verb- level, not root-level), so the same structural refusal applies to submit_up: if the cell PR's current head SHA equals the SHA the last pr_fail recorded, no new dev work landed on the cell branch -> the diff is byte-identical -> refuse, do not re-open the gate. Different SHA -> branch advanced -> allow. - _submit_up_unchanged_pr_guard mirrors _submit_root_unchanged_pr_guard (cell-PM remediation: re-delegate to the dev + wait for re-assembly), wired into submit_up after _submit_up_guard passes. - Renamed shared _current_root_pr_head_sha -> _current_pr_head_sha (both guards use it; the lookup was never root-specific). - Every ambiguous case FAILS OPEN (no prior fail, no recorded sha, no pr_number, no resolvable project, git/closed-PR None) — only the exact- unchanged case is hard-blocked. TDD red->green; ruff + mypy clean; F007+F016 guard suites green (15 passed). * [F008] evidence_builder: surface persisted pr_review verdict+issues in the PM task_handoff The pr_fail a2a steer to the owning PM is fire-and-forget; a PM respawned into needs_revision later read none of it (build_task_handoff never looked at notes_structured), saw a generic 'needs revision' with zero concrete change-requests, and re-submitted the same PR (the 2026-06-27 infinite pr_fail loop on 9980d0a0 / PR #138). The signal-gap was only partially closed by the a2a. build_task_handoff now extracts notes_structured.pr_review (verdict/summary/issues/head_sha — the slot pr_fail authors on every fail) into a pr_review field on the handoff, so every PM briefing for the task carries the concrete change-requests. A prior pr_fail alone now counts as prior-work-worth-resuming. Type-guarded + capped; absent => no key (no misleading empty slot). TDD red->green; ruff + mypy clean; evidence_builder suite green (14 passed). * [F009] notification: derive requires_ack from ACK_REQUIRED_BY_TYPE, not the True default NotificationService._create_notification built NotificationTable without requires_ack, so the column default (True) applied to EVERY notification - including informational REVIEW_REQUEST / DOCUMENTATION_REQUEST / A2A_REQUEST / KNOWLEDGE_SHARE (ACK_REQUIRED_BY_TYPE -> False) and every @mention from MessagingService._notify_mentions. Each false ack-required inflated the recipient's unacked set and soft-blocked i_am_idle into respawn churn. - _create_notification: requires_ack=ACK_REQUIRED_BY_TYPE.get(type, True) (unmapped types default True - preserve the action-required bias). - _notify_mentions: requires_ack=False explicit (MENTION is informational). TDD red->green (identity is False/is True assertions - the mocked flush doesn't apply SQLA's insert-time default, so pre-fix the attribute was None); ruff + mypy clean; notification suite green (18 passed). * [F010] notification: never dedup informational notifications (knowledge-share data loss) The purpose-based dedup suppressed a same-purpose (same sender/type/task, overlapping recipients) notification while a prior one was unacked. For informational types (KNOWLEDGE_SHARE / MENTION / A2A_REQUEST / BROADCAST + the pickup-proves-receipt triad) each send carries DISTINCT content (a new learning, a new mention) and acking is voluntary, so a recipient who never acks the prior one let the dedup permanently suppress every subsequent same-sender broadcast - silent learning-broadcast data loss. The dedup's anti-loop rationale (stop unacked-set inflation soft-blocking i_am_idle) only holds for action-required signals. Gate the dedup on ACK_REQUIRED_BY_TYPE.get(type, True): action-required types still dedup, informational types always create. Unmapped types default True (dedup on). TDD red->green; ruff + mypy clean; notification + dedup suites green (20). * [F011] playbook: de-index rejected/archived playbooks from the PLAYBOOKS RAG index * [F012] release_executor: fail-closed on git add/commit before push * [F013] release_proposal: Redis SET NX mutex guards the ~40min execute against concurrent approves * [F015] flow_qa/flow_doc: add i_am_blocked route (manifest-registered escape hatch was 404) * [F018] claim_guards: treat blocked as active + broaden the guard lookup so a blocked dev can't double-claim * [F019] git: clear orphaned .git/*.lock files after a timeout-SIGKILL'd mutation op * [F031] identity: role_for_slug_or_none so defensive skip-guards don't crash the dispatcher tick on stale slugs * [F032] test: unknown-assignee claim reaches release-to-pending path F031's role_for_slug_or_none fix made the unknown-assignee release branch in _dispatch_claimed_without_agent reachable (the human-only guard no longer raises/short-circuits on a stale slug). Lock that reachability in: a claimed task with an unknown-assignee UUID past grace returns the slug (not None) so get_agent_role -> 'unknown' releases the claim to pending for a role-matched reclaim. * [F033] orchestrator: capture container_id at startup re-adoption _readopt_running_agents registered re-adopted ACTIVE instances with container_id=None. _check_health skips container_id-is-None instances, so when a re-adopted container later exited the stopped-container handler never ran and the task stranded under a phantom ACTIVE instance forever. Add _resolve_container_id (docker inspect -f '{{.Id}}') and store the real id on re-adopt. Best-effort: a probe failure degrades to None (still ACTIVE; the reaper's Docker-liveness fallback covers it). * [F034] orchestrator: re-stamp respawn last_check at restore _pm_made_rule_following_retry bounds its tracing_gap audit lookup with since = record.get('last_check'). A stale persisted last_check from before the restart matched pre-restart tracing_gap rows, falsely resetting the breaker on the very first post-restart spawn — exactly when a fresh strike count should be evaluating current state. _partition_respawn_rows now re-stamps last_check to the restore time on every restorable entry, bounding the lookup to post-restart gaps only. * [F035] orchestrator: probe-resume loop actually revives parked agents _park_provider_unavailable parked the provider + offlined the instance but never registered a WaitingRecord, so _on_probe_success -> _parked_agents_for (always filtered on waiting_for=='rate_limit_lifted') returned [] and resolve_wait revived nobody — recovery fell to the 600s stale-claim reaper instead of the probe-success path the parking design relied on. Register + persist a rate_limit_lifted WaitingRecord at park time (mirrors mark_waiting_long, minus stop_agent — the container is already dead). Companion reaper guard: _reap_with_service now skips provider-parked assignees (_assignee_is_provider_parked) so the claim survives until the probe revives the agent — otherwise the reaper releases the claim to pending and probe-success respawns on a task the agent no longer owns. * [F036] orchestrator: read transcript for overload detection too The SDK server writes model-API errors (529/500/503) to /tmp/sdk-server.log, not stdout, so an overload marker can appear only in the durable Claude transcript — the same rationale already applied to the session-limit detector. _provider_overload_park_target read only docker logs, so an overload was missed and the agent crash-respawned straight back into it. Now concatenates the transcript tail before matching, mirroring the rate-limit path. * [F037] orchestrator: drop bare error-NNN overload markers The bare 'error 529'/'error 500'/'error 503' markers were broad enough to false-match an agent that merely writes about an HTTP status code in its own notes ('the endpoint returned error 500, retrying'), parking the whole Anthropic fleet on a non-issue. The SDK error formatter emits 'API Error: NNN' + a JSON error type, so the remaining 'api error: 529/500/503' + 'overloaded_error' + 'internal_server_error' markers cover every real overload without that false-match surface. * [F038/F039] orchestrator: sign X-Agent-Token on self-API calls The prior self-PATCH 401 fix only carried X-Agent-ID/X-Agent-Role. Arming ROBOCO_AGENT_AUTH_REQUIRED=true made the middleware require a signed X-Agent-Token, so every orchestrator self-call (auto-block / auto-resume / auto-recover / SLA annotation) 401'd and silently no-op'd — wedging paused/blocked parents. Add _system_api_headers() that wraps the base headers with a signed token for the system identity (issue_agent_token); switch all six self-call sites. Dev fallback: no secret set => UNSIGNED sentinel + auth not required. * [F040] orchestrator: finalize grok spawn session on cost-cap kill _enforce_grok_cost_budget killed + evicted the container without calling _finalize_spawn_session, so the open agent_spawn_sessions row stayed open (ended_at IS NULL) and the burned usage/cost was never recorded in the dashboard. Call _finalize_spawn_session(exit_reason='cost_cap') BEFORE popping the instance — it reads self._instances[agent_id] for the model + usage_session_id, which the pop would lose. * [F041] park grok exit-78 (auth missing/expired) instead of crash-retrying A one-shot grok container whose entrypoint ran grok_auth --check and found the token missing/expired exits 78 (EX_CONFIG). Crash-retrying 3x burns tokens for zero progress — the agent cannot start without a valid token. Park the provider with kind=auth_missing (same shape as the 429 exit-75 path) so the probe-resume loop revives the task once grok_auth.refresh_if_stale mints a fresh token; if still expired, the next exit 78 re-parks (no burn). Also fixes a latent F035 regression: _park_provider_unavailable now registers a WaitingRecord, so the bare-__new__ rate-limit park test had to set _waiting_records + stub _persist_waiting_record (mirrors the overload-test fixture). * [F042] isolate concurrent-duplicate conventions cache put in a savepoint Two task creates for the same project/HEAD can race to populate the conventions cache; the loser's INSERT fails the partial-unique index with IntegrityError. A bare session.add + flush poisons the shared session (the task-create transaction rides the same session), so every subsequent op raises 'this session is in error state' and task creation crashes. Run the INSERT in a savepoint (begin_nested) and swallow the IntegrityError: only the savepoint rolls back, the outer transaction stays usable, and the winner's row satisfies the next _cache_get. * [F043] guard escalate_up against resurrecting terminal tasks escalate_up had composes=() and no source-status guard, so a PM could escalate a COMPLETED/CANCELLED task and apply_escalation set it back to BLOCKED — bypassing the state machine's terminal-state invariant. Defense in depth: - spec: add PRECONDITION_NON_TERMINAL to escalate_up's extra_preconditions so the lifecycle gate rejects terminal tasks (invalid_state) before the journal:decision write fires; generalize _check_intent_preconditions to honor non-tracing rejection_kind (not_authorized / invalid_state). - service: apply_escalation (the single write primitive) returns False and refuses to mutate a terminal task — covers the HTTP escalate route which bypasses the spec gate. escalate() / escalate_up_to_role() return None on refusal so the gateway emits a clean invalid_state envelope. - route: the HTTP escalate route 409s a terminal task BEFORE sending the escalation notification (so a finished task isn't yanked back, PM not pinged). * [F044] pr_pass gate remediation points the reviewer at pr_fail, not i_am_blocked The pr_pass gate runs the toolchain + conventions guards on the REVIEWER's workspace, but their remediation text said 'call i_am_blocked' — a verb the PR reviewer does not have. The reviewer would chase a verb they cannot call instead of rejecting the PR. Make the guards reviewer-aware: a reviewer=True flag (passed by _pr_pass_blocked) switches the remediation to pr_fail(issues=[...]) — the reviewer's reject lever, sending the PR back to needs_revision for the dev to fix the environment / validator. The dev (i_am_done) path keeps i_am_blocked, which a dev does have. _conventions_guard (the pr_pass path) now passes reviewer=True through to _conventions_rejection. * [F045] rate-limit: loud activate-failure log + in-memory orphan-probe fallback The in-verb i_am_blocked(rate_limited) path wrapped RateLimitStateTracker.activate in a bare contextlib.suppress. A silent activate failure stranded the fleet: agents were parked in _waiting_records but the provider never entered the tracker, so the tracker-driven _sweep_rate_limit_probes never probed it and no _on_probe_success ever resumed them — parked agents stuck in WAITING_LONG. Fix: (1) replace the bare suppress with a try/except that logs an error event naming the provider + affected agents; (2) in _sweep_rate_limit_probes, after probing the tracker-listed set, scan _waiting_records for any rate_limit_lifted provider the loop did NOT cover and probe it via the time-expiry fallback (empty state -> probe now) so _on_probe_success resumes the parked agents. The fallback reads only local memory, so it still resumes when Redis was down at park time (list_rate_limited_providers failure now falls through to the orphan scan instead of returning early). * [F046] pr_gate: guard None runner result on concurrent transition (pr_pass/pr_fail) _gate_decision dereferenced the verb-runner result without a None guard. run_intent returns None when a concurrent transition (cancel or a racing reviewer) moves the task out of awaiting_pr_review between the precondition gate and the runner's final composed action (the verb runner's documented last-action source-status contract). The subsequent t.assigned_to / t.status / _post_gate_review_to_pr(t, ...) dereferences then crashed the gate with a 500 AttributeError. Add a None guard that surfaces a clean invalid_state rejection (re-fetch + re-issue) before any dereference; no PR post or a2a runs against a None task. TDD test_pr_gate_notifies_pm.py (+2). * [F047] conventions: reviewer-aware block-finding remediation on pr_pass gate The pr_pass (reviewer) conventions guard reused the dev-path block-finding remediation: 'add a waiver to .roboco/conventions.yml in your branch'. A pr_reviewer does not own the assembled cell->root / root->master branch and has no commit verb on it, so the waiver remediation is unreachable — a false positive stranded the gate with no self-recovery (the reviewer could neither commit a waiver nor pr_pass). The fail-open content path is documented precision-over-recall and stays as-is; the actionable gap is the remediation. Fix: _conventions_rejection now branches the block-finding remediation on reviewer=True (mirroring the could_not_run branch from F044). The reviewer path points at pr_fail carrying the findings as issues so the PR returns to needs_revision and the DEV fixes the violation or commits the waiver (the dev CAN commit to the branch); waiver authorship is framed as the dev's action, not the reviewer's. Dev i_am_done path wording unchanged. TDD test_conventions_gate_pr_pass.py (+1). * [F048] notify: reject human-only recipients (prompter/secretary) — no agent ack path notify() only checked the SENDER role. The recipient was resolved by NotificationService._resolve_recipients, which drops only unresolvable slugs — it does not exclude human-only roles. The prompter (intake-1) and secretary (secretary-1) are seeded agent rows, so they resolved, and an ack-required ALERT addressed to them sat permanently unacked (no agent auto-acks it), polluted the panel's pending-ack view, and — via the dedup query's ~acked_by.contains — permanently suppressed any later same-purpose notification from the same sender to that human role. The knowledge-share path already excludes all three human-only roles; the general notify path did not. Fix: a recipient-role guard in notify() via _reject_disallowed_recipient (folds the new check into the existing CEO-dependency-block return slot so notify stays under the PLR0911 return limit). Rejects prompter/secretary with not_authorized; the CEO is human too but acks via the panel, so it stays an allowed recipient (its only disallowed case, a dependency-block page, is preserved). TDD test_notify.py (+3: reject prompter, reject secretary, allow CEO). * [F049] merge_pull_request: idempotent on already-merged PR (mirror _merge_with_retry) * [F050] merge_pr_for_task: verify caller pr_number matches task's recorded PR * [F051] open_conventions_pr: refuse dirty tree + verify checkout-base landed * [F052] pr_target: scope task lookup by project_id (mirror close_pull_request) * [F053] _token_for_project: log decryption failure (key rotation) with project slug * [F054] learnings index: enforce shareable on every shared retrieval path (private-leak fix) * [F055] messaging: recover from concurrent channel auto-create race via savepoint + re-fetch * [F056] messaging: lock group row before session check-then-create to prevent active-session orphan race * [F057] playbook: index/unindex as a post-commit step so the RAG corpus never leads the status transaction * [F058] release-readiness: non-empty bump plan on first release _canonical_bump_files derived the bump set from the previous chore(release): commit. On the first release there is no such commit, so it returned [] -> assess set version_bump_plan=[] -> the executor published a tag with no files bumped (a no-op masquerading as X.Y.Z). Fall back to the version-reference scan when no prior release commit exists: the files currently embedding the version are exactly the set a first release must bump, and the set the first release commit then records as canonical for subsequent releases. Read-only derivation; the CEO-approval gate and fail-closed executor are untouched. * [F059] self-heal: hold fix tasks for CEO Approve-&-Start (restore dispatch gate) The module docstring promised self-heal fix tasks 'wait for the CEO's Approve-&-Start', but _originate created them confirmed_by_human=True and the orchestrator dispatched them at once — a self-heal fix that re-broke CI would trigger another cycle, open another auto-dispatched fix, and loop with no CEO gate on dispatch. Restore the documented gate: * _originate opens the task confirmed_by_human=False (held for the CEO). * The orchestrator holds a self-heal task out of both the PM and dev dispatch paths until confirmed_by_human flips True. * approve_and_start (the CEO's start gate) sets confirmed_by_human=True so the held task finally dispatches (idempotent for board/intake tasks already True). * list_pending_for_agent scopes the give_me_work hold to self-heal (source != self_heal OR confirmed_by_human) so an already-alive PM can't grab it pre-approval — while ordinary delegated subtasks (confirmed_by_human=False by default, where the delegation IS the authorization) still dispatch. The 'never self-deploys' guarantee (no merge) is unchanged. * [F059] fix DB-integration test auth + retype self-heal root code→planning conftest test-DB defaults matched the project's own running postgres (roboco/roboco @ localhost:15432, the docker-compose roboco-postgres service with CREATEDB) instead of the OS user on localhost:5432 which has no such role — every db_session test failed with InvalidPasswordError instead of running. Once the DB connection worked, the self-heal origination DB test went RED with MAIN_PM_NO_CODE: the self-heal root was task_type=CODE owned by main_pm, the combo the main_pm_cannot_own_code guard rejects. The Main PM coordinates the fix (delegates the code work to a cell dev); it has no code verb. Retyped CODE→PLANNING and rewrote description/AC to coordination-level. * [F060] emit reversal audit row on claim-branch-failure rollback The forward task.claimed audit row is flushed before the branch-creation attempt, and AuditService commits on its own connection, so the rollback's flush reverts the task row but not that audit row — the journey's last event stayed task.claimed while the task reverted to its pre-claim status, diverging from real state and corrupting downstream cycle-time/bottleneck metrics. The rollback now emits a CLAIMED->original reversal audit row (only when the forward transition was made) attributed to the claimant. * Removing completely unnecessary files (for the repo they are unnecessary) * [F061] audit status-transition rows now written in-session (F061/F073/F075) _emit_status_transition_audit now writes AuditLogTable rows into self.session synchronously (session.add) instead of dispatching AuditService.log_task_event fire-and-forget on its own connection. The audit row now commits/rolls back atomically with the status transition in the caller's transaction, closing three facets at once: - F061: audit commit no longer decoupled from the transition commit - F073: a committed transition can no longer have NO audit row (the row rides the same transaction; a swallowed persist can't drop it) - F075: a transition rolled back inside a verb savepoint no longer leaves a phantom audit row (the row is in the savepoint too) log_task_event is now called only from this helper (narrow blast radius verified); revision_count increment stays at this single chokepoint. Cycle-time/bottleneck reconstruction from task. events is no longer silently corruptible. Tests: test_emit_status_transition_audit_writes_in_session_atomically, test_finalize_claim_rollback_emits_reversal_audit, escalation-audit tests retargeted to in-session AuditLogTable rows. Also: _canonical_bump_files grep-looseness follow-on (F058) -- filter by subject, not body; git log --grep matches any message line, so a non-release commit whose body references chore(release): shadowed the real release commit. Test test_canonical_bump_files_ignores_body_only_chore_release_match. * [F061] drop type:ignore from audit-emit tests Convention: no type:ignore/noqa. The F061 in-session audit-emit tests used '# type: ignore[assignment]' to assign a MagicMock to AsyncSession.add, and the F060 test assigned to .flush the same way. Rewritten to hold a local 'session: MagicMock' variable (mypy sees its auto-children as MagicMock, so .add.side_effect / .flush assign cleanly with no suppression). Verified via 'mypy tests/' that both files are now type-clean (the F060/F061 commits had skipped tests/ in mypy, masking two method-assign errors). * [chore] clear all 64 pre-existing mypy errors in tests/ (no type:ignore) Convention: no type:ignore/noqa, and pre-existing violations still violate. The make-quality gate runs 'mypy roboco/ tests/', but the prior commits' gates only ran mypy on production files, masking 64 type errors across 15 test files (method-assign, unused-ignore, no-untyped-def, attr-defined, union-attr, has-type, index, misc). Fixed without any type:ignore: - method-assign (svc.session.X = / svc.method = AsyncMock()): hold a local 'session: MagicMock'/'AsyncMock' and assert on it, or stub via object.__setattr__ / monkeypatch / a typed '_bind' helper returning Any, or alias 'cc: Any = c' (the pattern the file already used). - unused 'type: ignore[assignment]' (real code was method-assign): removed; replaced with the no-suppression patterns above. - 'Callable[...] has no attribute assert_*': keep a typed local ref to the AsyncMock and assert on the local, not the method-typed attr. - no-untyped-def: annotate helper params (Any / pytest.MonkeyPatch). - attr-defined / index / union-attr: type the helper as Any, narrow with an 'is not None' assert, or add the missing attr to a fake. - has-type / return-value: fix the declared return type to the tuple the function actually returns. - PLC0415 inline imports: hoisted to top-level. test_pr_gate_notifies_pm._stub_gate_path converted fully to the 'cc: Any = c' alias (it already used it for one attr) so its five '# type: ignore[method-assign]' suppressions are gone. mypy tests/: 64 errors -> 0 (538 files). ruff check tests/: clean. All 84 tests in the touched files pass. * [chore] remove all remaining type:ignore suppressions from tests/ Converts 115 `# type: ignore[...]` suppressions across 23 test files to no-suppression patterns (helper-return widening to Any, local Any aliases, cc:Any aliases, cast at narrow call sites, typed fixtures) so the hard no-type:ignore convention holds across tests/. No test logic or assertions changed — only mock-wiring mechanics and type annotations. Gate: ruff check tests/ clean; mypy tests/ (538 files) clean; 176 changed-file tests pass. Zero real suppressions remain (the 7 grep hits are 3 hygiene- checker string-literal test inputs and 4 prose mentions in comments). * [F062] work_session.merge_pr: idempotency + active-status guard merge_pr unconditionally set pr_status=merged, pr_merged_at, merged_by, status=COMPLETED on whatever session it loaded — the only session-terminal transition in WorkSessionService lacking both the active-status guard (complete/abandon) and the terminal-idempotency guard (close). Two failure modes: (1) a retried merge after a successful-but-unconfirmed GitHub merge overwrote merged_by/pr_merged_at with the retry's actor/timestamp, corrupting the merge audit trail; (2) merge_pr on an ABANDONED session resurrected it to COMPLETED, undoing the single-active abandonment. Mirrors close()'s guard: if status != ACTIVE, return the session unchanged. Both git.py callers await merge_pr and discard the return, so the no-op is safe. TDD: 3 tests (happy-path + both modes). * [F063] workspace._clone_repo: rmtree half-configured clone on failure If _configure_git raised CalledProcessError before its `remote set-url` scrub, .git/config kept the tokenized auth URL (the project PAT) and _assert_no_pat_leak never ran. The except clauses raised WorkspaceError without removing the workspace, so the next ensure_workspace's health short-circuit (valid .git with HEAD + objects) skipped past the leak — mounting the agent on a workspace whose .git/config let it read+exfiltrate the PAT. Both clone-failure except clauses now rmtree the workspace before raising, so a half-configured clone is destroyed and ensure_workspace re-clones from scratch. TDD: 2 tests (configure-failure leak + timeout). * [F067] flow_main_pm: add missing /triage route main_pm's manifest advertises triage (lifecycle.intents_for_role(MAIN_PM) includes it via _PM_ROLES, alongside triage_all) but flow_main_pm.py had no POST /triage route, so a main_pm agent calling triage hit a raw 404 that bypassed the per-verb circuit breaker. Added the route mirroring flow_cell_pm's /triage — wires to the existing team-scoped choreographer.triage (uses pm.team, works for any PM role; Main PM gets its own team's blocked/awaiting tasks). Fix direction: add-route, NOT remove-from-manifest — the manifest is spec-correct (intents_for_role by construction); removing triage would contradict the spec and leave main_pm with only cross-team triage_all. TDD: test_triage_route_exists_and_dispatches. * [F068][F069] mcp servers: classify all rejection shapes + envelope 404s F068: the do/flow-server circuit breaker only counted rejections whose `error` field was a STRING in _CIRCUIT_REJECTION_KINDS. A 422 validation failure (no `error` field, a `detail` list) and a 500/HTTPException (dict-shaped `error` from the exception handlers) both bypassed the breaker → unbounded retries on a storm of either. Added _classify_rejection(payload) (shared, applied to both servers) mapping all three shapes to a counted kind: string error (existing), dict error → substring-mapped code (*DENIED*/*AUTHORIZED*/*FORBIDDEN*/*PERMISSION*→not_authorized, INVALID_INPUT/*VALIDATION*→incomplete_input, *NOT_FOUND*→None parity, else →invalid_state), 422 detail→incomplete_input. The dict TypeError defence lives in the classifier (isinstance, never dict-in-frozenset). F069: a manifest-registered verb whose HTTP route is missing got FastAPI's raw `{"detail":"Not Found"}` 404 body — a non-envelope payload the breaker couldn't classify, so a storm bypassed it. _post now synthesizes an invalid_state Envelope rejection (with a remediate hint → i_am_blocked/i_am_idle) for a 404 status, routed through _record_and_check_circuit so the breaker counts it. A 404 that carries a real Envelope (error field present) is surfaced as-is, preserving test_flow_post_returns_envelope_on_404. TDD: 422/dict/404 tests in both server test files; updated test_dict_shaped_error_does_not_crash to assert the SDK is now called with not_authorized (replacing the pass-through assertion that encoded the bug). * [F064][F065][F066] websocket: non-blocking fan-out, finally-disconnect, idle timeout F064: the bridge forwarder awaited every conn.send_text in a gather with no per-connection queue and no send timeout — one slow WS client back-pressured ALL event delivery to ALL clients (head-of-line blocking on the listen loop). Each connect_* now registers a _ClientConnection (bounded asyncio.Queue(256) + sender task); broadcasts enqueue via put_nowait (drop + structlog warn on QueueFull) and return immediately. The sender drains the queue with each send wrapped in wait_for(SEND_TIMEOUT=10s). Unregistered legacy sockets (set directly into a subscription set, bypassing connect_*) get a timeout-bounded fallback send task held in _pending_sends (ruff RUF006). disconnect cancels + drops the sender. F065: route handlers caught only WebSocketDisconnect with no finally — a non-clean exit (anyio closed-resource, CancelledError, transport error) propagated without manager.disconnect, leaking the dead socket into every subscription set forever. Added finally: manager.disconnect(websocket) to all 5 handlers (disconnect is idempotent). F066: no server-side heartbeat/idle timeout — a half-open socket from a dead container blocked receive_text forever and was never reaped. receive_text now wraps in wait_for(IDLE_TIMEOUT_SECONDS=90s); on TimeoutError, log + fall through to the F065 finally. Named module constants (no config.py precedent for WS tuning; callers/tests patch them). TDD: 22 new tests across 3 files (handler cleanup, idle timeout, send queue), non-flaky across repeats; 1 existing test adapted with a yield for the new async fan-out (assertion unchanged). ruff/mypy clean, 421 unit/api tests pass. No type:ignore/noqa. * [F022][F023][F024][F025][F026] api: scrub secrets from 422 log, gate a2a/dashboard/orchestrator routes, SSE session-per-query - middleware: redact known credential fields (git_token/api_key/token/...) from the 422 request-validation log line; response body unchanged - a2a: require_any_authenticated_agent on /message/send + /message/stream; subscribe_to_task opens a short-lived session per poll instead of holding one asyncpg connection for the full SSE lifetime (pool exhaustion) + auth - dashboard: gate auditor flag/report mutating routes to Auditor or CEO - orchestrator: router-level CEO gate on all control routes (spawn/stop/...) TDD; ruff/mypy clean; 449 unit/api tests green; no type:ignore/noqa. * [F030] conventions: typescript-scoped custom rules now apply to .tsx files The validator tags a .tsx file as language 'tsx' (the JSX grammar needs that tag, distinct from plain 'typescript'), but a custom rule scoped to 'typescript' — the language the scan reports for a React+TS repo — silently skipped every .tsx file. The two suffix maps were NOT unified: the 'tsx' tag is load-bearing (grammars.py picks the JSX grammar on it; hygiene.py keys on it), so unifying would make .tsx fail to parse. Fix is in check_custom: a one-directional dialect map _DIALECT_OF = {'tsx': 'typescript'} — a typescript-scoped rule fires on a .tsx file, but a tsx-scoped (JSX-only) rule still does not fire on plain .ts. TDD; ruff/mypy clean; 80 unit + 38 integration conventions tests green. * [F029] websocket: remove broken /api/permissions/check loopback from channel stream channel_stream called validate_channel_access, which HTTP-loopbacked to GET /api/permissions/check — a route that does not exist. Every call 404'd -> False -> the channel stream closed with WS_1008_POLICY_VIOLATION for EVERY client, so the real-time channel stream was dead. Removed the function, its call site, and the now-unused httpx + settings imports. Post-F004 the panel-token gate is the channel-stream authorization (the CEO panel is the sole WS client and may view every channel), so the broken loopback is removed rather than replaced with an in-process check the CEO always passes. The legitimate enforcement.validate_channel_access (slugs, in-process static ACL) is a different function and is untouched. F027 is resolved-by-F004 (no code change): all three per-agent streams gate on _require_panel_token first, so only the authorized CEO panel can connect — 'any viewer subscribes to any target' is closed. TDD; ruff/mypy clean; 530 unit/api+enforcement+RBAC tests green. * [F078] release_executor: deadline every subprocess (git/make/gh/clone) A hung git/make/gh/clone would block the CEO-gated release loop indefinitely. Wrap each proc.communicate() in asyncio.wait_for via a shared _await_proc helper; on expiry proc.kill() the child and return a non-zero rc (124) so every caller's fail-closed branch fires. Mirrors the quality-gate _run_one kill-on-timeout idiom. Deadlines are generous (30min gate / 10min clone / 5min push+gh) so a legitimate slow op is never wrongly aborted — floor-assertion tests pin the floors to guard exactly that logical regression. Green path returns the real rc unchanged. * [F072] reaper: deadline docker inspect/exec + harden _check_health sweep A hung Docker daemon (or a stuck container FS) froze the single asyncio event loop: the reaper runs inline before every dispatch tick and shares that loop with every background sweeper. Bound each docker subprocess with asyncio.wait_for; on expiry proc.kill() the child and either raise (inspect / resolve_container_id — callers apply their own fail-direction) or return None (the gateway probe — inconclusive, caller declines to act, matching its existing probe-failure contract). Deadlines generous (10s inspect / 30s exec) so a legitimate slow docker call is never wrongly aborted; floor-assertion tests pin the floors. Also harden _check_health's per-agent loop so one agent's hung inspect skips that agent, not the whole sweep — preserving the check-all-agents invariant the timeout-then-raise would otherwise break (without this, a hung daemon means no agent gets health-checked any tick). * [F076] say/dm: handler guard rejects all 4 no-comms roles, not just auditor The say()/dm() defence-in-depth guard only rejected auditor, but CLAUDE.md mandates the same no-agent-comms invariant for pr_reviewer (posts findings on the PR), prompter and secretary (human-only, note + evidence). For those three the manifest was the only gate, so a call bypassing the manifest (direct API POST, test harness, future routing change) would not be refused at the handler — admission depended on the agent's slug happening to be absent from the channel/a2a matrix. Extend the guard to a _NO_COMMS_ROLES frozenset (auditor + pr_reviewer + prompter + secretary), matching the explicit role-frozenset gates on commit/notify/pitch/playbook/open_session. Role-appropriate remediation per role. The claimed defence-in-depth now covers 4 of 4 silent roles, not 1 of 4. * [F070] drain fire-and-forget _bg_tasks on shutdown (bounded, data-preserving) Orchestrator.stop() cancelled only the named loop tasks + agents, then returned, abandoning in-flight _schedule_bg work. An in-flight _persist_respawn_record upsert dropped at shutdown meant the last few gate-mutation strikes never reached the DB; restore_respawn_tracker() on the next start repopulated a stale lower count and the dispatcher re-burned the full 4-spawn strike threshold against a still-wedged task — the exact re-burn the durable tracker exists to stop. Audit-log writes (load-bearing for cycle-time/rework metrics) were similarly dropped. Add _drain_bg_tasks(): bounded wait (5s default) lets short DB writes commit before exit (data preserved), then cancels any stuck task past the deadline so a hang can't wedge shutdown. return_exceptions=True so one failing bg task doesn't crash the drain. Wrap the stop_agent loop in try/except + logger.exception so one bad agent can't skip the drain (re-introducing the data-loss tail). Floor test pins the deadline >= 3s so a too-short change can't silently drop a legitimate slow write. * [F071] abort non-blocking intake/secretary spawn on mid-spawn shutdown The non-blocking spawn (start_intake_session / start_secretary_session) schedules _spawn_intake_container_guarded / _spawn_secretary_container_guarded via _schedule_bg. Those run docker run and only register in _instances at the END. If shutdown arrived between docker run and the registration line, the container was started but the orchestrator had no handle — stop() iterates only _instances, so the container was orphaned (leaked, manual docker rm). Worse, the F070 drain could let the spawn coroutine complete the registration AFTER stop() already iterated _instances, landing a live container into a shutting-down registry nothing tears down. Add a post-docker-run shutdown guard in _spawn_intake_container and _spawn_secretary_container: re-check self._running after _run_container_cmd returns; if the orchestrator began shutting down, remove the just-started container (by its deterministic name) and raise _SpawnAbortedDuringShutdown WITHOUT registering. The guarded wrappers catch that BEFORE except Exception and close the live relay silently (shutdown is not a user-facing failure, no error pushed to the SSE stream). The F070 stop() drain awaits the bg spawn coroutine, so the abort surfaces cleanly. TOCTOU-safe: between the _running check and the _instances assignment there is no await (config + instance construction are sync), so once the check passes, registration completes before the event loop can interleave stop(). The normal running path is unchanged (sanity tests pin it). * [F074] per-agent advisory lock closes claim TOCTOU _run_claim_guards read the agent's other tasks via unlocked SELECTs before claim() took its row lock, and claim()'s FOR UPDATE locked only the TARGET row — so two concurrent i_will_work_on by the SAME agent on TWO DIFFERENT pending tasks each locked their own row, each read an empty in_progress set, each passed already_active, each claimed+started → the agent ended with two in_progress tasks (the in-process asyncio Lock is lost on orchestrator-restart split-brain, so it wasn't a DB-level guarantee). Fix: TaskService.acquire_claim_lock takes a transaction-scoped pg_advisory_xact_lock keyed by hashtextextended(agent_id). The gate acquires it BEFORE the guard reads (for non-coordinator roles only) so the second concurrent claim's read sees the first's committed in_progress task and is rejected. Tx-scoped → auto-releases on commit/rollback, can't outlive the request. Coordinator exemption (the key logical-regression guard): cell_pm / main_pm do NOT take the lock — the PM coordinator concurrency feature lets a PM plan+delegate many roots in parallel, and a per-agent lock would serialize those claims and regress it. Matches the existing _COORDINATOR_ROLES already_active/paused guard exemption. A hash collision only causes benign false serialization, never a false negative. Tests: unit (dev acquires lock before guard read; coordinator does not) + real-PG integration (same-agent serializes, different-agent does not, releases on rollback). * [F021] handle SSE transport errors so the intake composer isn't stuck openStream registered listeners for the server-sent event kinds but not the EventSource's own transport-level error. The 'error' kind IS in LIVE_EVENT_KINDS, so a server-sent event:error (JSON MessageEvent) was handled — but a dropped connection / dead session fires a plain Event with NO data, which JSON.parse(undefined) swallowed in the try/catch, so the stream 'stayed open' (EventSource loop-reconnected a session that no longer existed) and isSending stayed true — the composer was permanently disabled. Fix: route the 'error' event by payload. A MessageEvent with string data is a server-sent error → handleEvent (unchanged). A no-data Event is a transport error → handleTransportError: clear streamingId/activity, set isSending false, add a 'connection lost' error message, keep a draft/batch preview up (so the human can still act on a proposed card) else land on 'chatting', and close the dead stream so EventSource stops loop-reconnecting. Tests: renderHook + a jsdom EventSource double that fires a transport error (plain Event, no data) vs a server-sent error (MessageEvent + JSON). RED: transport error left isSending true; GREEN: resets to false, surfaces the message, closes the stream. The server-sent-JSON path is unchanged. Full panel suite (129) green; eslint/typecheck/prettier clean. * [F081] Approve dialog: label notes required (>=20 chars), not optional The CEO Approve dialog's notes label fell into the default branch ('Notes (optional') for the approve action, but approve actually requires substantive notes >= 20 chars — enforced client-side (toast error on < 20) and server-side. So the CEO was told 'optional' and only learned the real requirement from a toast after hitting submit with empty notes. approve and start both require >= 20 chars; reject only requires a reason. Collapse the label to two branches: reject -> 'Reason for rejection (required)'; everything else (approve + start) -> 'Approval notes (required, >= 20 characters)'. The approve placeholder now also signals intent ('Why this is ready to ship...'). Tests: render the queue, click Approve, assert the notes label says 'required' + '20' and does NOT say 'optional'. RED: label read 'Notes (optional)'; GREEN: 'Approval notes (required, >= 20 characters)'. eslint/typecheck/prettier clean. * [F082] surface release-proposal query failures instead of silent hide The card collapsed any non-404 backend failure (500 / network drop) onto `!proposal` and returned null, so the CEO had no idea the release-proposal endpoint was unreachable. Distinguish the cases: isError + a Retry affordance vs the 404 null empty state that stays hidden. Mirrors PrReviewQueue. * [F083] clear stale usage snapshot when /ws/system leaves connected The hook synced wsState into the store but never dropped usageData when the stream dropped, so on reconnect wsState flipped to "connected" before any fresh USAGE_SNAPSHOT arrived and UsageOverviewPanel rendered the prior session's totals/cost as if they were live. Clear usageData whenever state leaves "connected" so the panel falls back to the polling summary until a new snapshot lands. Connected->connected is a no-op clear skip. * [F084] scope per-control disable to the in-flight mutation, not all FeatureFlagsCard disabled every switch while any one flag toggle was pending, and PlaybookReviewQueue disabled every row's Approve while any one approve was pending — so the operator couldn't act on an independent control during a slow round-trip. Gate the disable on the in-flight mutation's variables (matching key / id) so only the control being mutated locks; the others stay usable. The same-flag double-tap protection is preserved. * [F085] reject submitting both project_id and product_id validate() only checked 'at least one of project/product', so the dialog let both be submitted together. The server silently lets product_id win at routing and drops project_id, recording a misleading, never-used repo. Add a validator that refuses the ambiguous submit with a clear error. The at-least-one rule and the single-pick submit paths are unchanged. * [F020] kanban: confirm admin-override drags that skip lifecycle preconditions A drag on the operator kanban routes the status move through the admin status-override, which bypasses the in-band lifecycle validator entirely. That override is intentional (it's how an operator recovers a wedged task) but it also let a careless drag skip material preconditions silently — completing a task with no open PR, QA-bypassing, finishing docs on a task whose docs aren't complete. Leave the override intact but make the bypass explicit: compute the preconditions the dragged move would skip (open PR, docs complete, self-verified + commits + progress for submit-qa, visible non-terminal subtasks for coordination-root targets) and, when any are skipped, hold the move behind a confirmation dialog that lists exactly what's being skipped. Precision over recall — only warn on what the panel can verify from the task and its in-list children; never fabricate a 'satisfied' claim, and stay silent on benign transitions that gate on nothing we can check. The admin status-override capability is preserved (Confirm still fires it); this only surfaces the bypass instead of letting it happen silently. Does not touch the master-merge invariant — the board's updateTask is the operator override, not the Main-PM merge path. * [F086] prompter: restore parked cell content on project toggle off/on rebuildCellWork appended a blank {summary:'', items:[]} entry for a newly- selected cell, so toggling a cell's project OFF then back ON in the MegaTask review card discarded the agent-authored per-cell summary/items — the entry was dropped on toggle-off and re-added blank on toggle-on. Park each draft's last per-cell content in client-only BatchProposal state (parkedCellWork, keyed by draft index — never sent to the backend; confirm ships only title/drafts/project_ids/route, and it ride-alongs into the localStorage persist slice so the restore survives a reload mid-review). rebuildCellWork gains an optional priorByCell map: a re-added cell with no live entry restores its parked summary/items (with the new project_id) in- stead of blanking; a live entry still wins over a stale parked copy so an in-place edit is never regressed. parkCellWork is the pure merge seam (prevParked seeds, live work overwrites) the setBatchDraftProjects updater calls — kept pure so the updater stays a thin caller. Tests: rebuildCellWork restore/blank-fallback/live-wins + parkCellWork retain/overwrite/merge (6 new), 19 GREEN. eslint/typecheck/prettier clean. No wire-payload change, no regression to the fill/drop/one-repo-per-cell invariants. * Updated domain * [F087,F088] enforce panel token on live-chat bridges (Phase 5) Add a CEO-bound, header-token-only gate (require_panel_token) at the route level of the prompter_live + secretary_live bridges, which were the only panel-facing API surface that ran unauthenticated. It mirrors the WS _require_panel_token and _check_agent_auth_token contracts: in dev (ROBOCO_AGENT_AUTH_REQUIRED unset) a missing token is allowed; a presented-but-forged token is rejected even in dev; in prod nginx already injects the CEO-signed X-Agent-Token on /api/ for GET + POST, so the SSE stream (EventSource can't set headers) and the POSTs are now checked instead of anonymous. Applied to start/stream/status/messages/stop on both routers; preview_live_batch switched from CurrentAgentContext+noqa to the route-level gate (genuinely auth-only). confirm/confirm-batch/re-interview keep CurrentAgentContext (they use agent.identity). The container->relay /events callback is intentionally left ungated (internal Docker network, opaque session id) — gated by a test sentinel so Option B (spawn+SDK token wiring) is a deliberate future decision. No panel/nginx/spawn/SDK changes; master merge invariant untouched. 22 new TDD auth tests, 492 api tests green. * [F089] honest WorkSession agent_id nullability across the read path The work_sessions.agent_id column is nullable=True with ondelete=SET NULL — deleting an agent nulls the FK on every session it ever held. The ORM annotation lied (Mapped[UUID] non-optional), the converter papered over the lie (typing_cast to a non-optional UUID), and the response model rejected None outright (WorkSessionResponse.agent_id: UUID). A session whose agent had been deleted crashed the GET endpoint with a pydantic ValidationError instead of serializing agent_id: null. Make the read path honest end-to-end: - WorkSessionTable.agent_id: Mapped[UUID | None] (matches the column). - WorkSessionResponse.agent_id: UUID | None (serializes null, no crash). - session_to_response passes agent_id via typing_cast('UUID | None', ...) to bridge SQLAlchemy's UUID[Any] to stdlib uuid.UUID while preserving None-ness (the cast stays for the same mypy-plugin reason every other field uses one; it no longer narrows away None). WorkSessionCreate.agent_id stays UUID — at create time the claiming agent is always known. The unused WorkSession pydantic read model is left as-is (never materialized from a DB row). task.py:_needs_revision_dev already None-guards ws.agent_id via to_python_uuid (returns None -> skip). * [F090] drop auditor from write_roles on main-pm-board / board-private The auditor is a silent, read-only observer on every channel, but the channel catalog (roboco/foundation/policy/communications.py) listed it in write_roles for main-pm-board and board-private 'for parity' with the legacy CHANNEL_ACCESS table, while the actual silent-observer rule was enforced only at the say/dm guard (content_actions._NO_COMMS_ROLES) and PermissionService.can_write_channel's auditor short-circuit. That left the catalog-only enforcement path — the HTTP messaging route (messages.py send_message -> validate_channel_access) — authorizing an auditor write that both the say/dm guard and PermissionService would have blocked. A reader of the catalog also believed the auditor could post to those channels, which is false. Fix: remove Role.AUDITOR from write_roles on both channels (main-pm + board remain writers; ceo remains a writer on board-private). The auditor stays in read_roles, so its silent read is unchanged. silent_roles is left empty (matches the announcements precedent: auditor reads via read_roles, not the silent bucket) — the DB seed and silent_observers field are untouched. Logical-regression check: the auditor's read access on both channels is byte-for-byte preserved (still in read_roles, so validate_channel_access read returns True via the direct list); the legitimate writers (main-pm, product-owner, head-marketing, ceo) are untouched; CHANNEL_ACCESS is derived from the spec so the foundation/seed drift tests self-adjust; PermissionService.can_write_channel already short-circuited auditor to False everywhere, so no behavior change there; AUDITOR_SILENT_ACCESS is unchanged (auditor not added to silent_roles -> no DB silent_observers change -> no group-access behavior change); the say/dm _NO_COMMS_ROLES guard is unchanged. Tests: 3 new in test_channel_access.py — auditor write on main-pm-board/board-private now raises ChannelAccessDeniedError (RED before: returned True), auditor read still True, main-pm/ceo still write. * [F091] warn at spawn time when host grok auth.json is missing GrokCliProvider._append_grok_auth_mount silently skipped the mount when the host ~/.grok/auth.json was absent. The spawn still succeeded (docker run returned 0 — the container was created), so the operator had no spawn-time signal that the agent was doomed: the entrypoint's `python -m roboco.llm.providers.grok_auth --check` backstop then refused to start (exit 78) and the failure only surfaced later via the container's log markers. Fix: emit a spawn-time WARNING (module logger) naming the missing file and the remediation (`grok login` on the host, or set ROBOCO_HOST_GROK_DIR) when the mount is skipped. The spawn outcome is unchanged — the container still starts and the existing exit-78 -> park flow (F041) still catches it — but the operator now sees the missing credential immediately instead of diagnosing a later exit-78. Logical-regression check: the mount-present path is byte-for-byte unchanged (auth.json exists -> the -v bind is appended, no warning); the spawn still succeeds when auth is absent (no raise — the existing test_grok_spawn_omits_auth_mount_when_absent still passes: no mount, no crash); the exit-78 entrypoint backstop and the orchestrator's exit-78-park handling (F041) are untouched; a module-level logger adds no side effects. Tests: new test_grok_spawn_warns_when_auth_absent uses caplog to assert a WARNING mentioning auth.json + `grok login` is emitted on a missing-credential spawn (RED before: no warning; GREEN after). 102 grok tests green; ruff/mypy clean. * [F092] decode JWT exp when refresh omits expires_in xAI's refresh-token response sometimes omits expires_in. Without it the new access token kept the stale pre-refresh expires_at, so is_valid / --check forever rejected a fresh token — and the refresh loop re-rotated the single-use refresh token every tick, killing the credential (F006). The access token is a JWT whose exp is the authoritative expiry: decode it when expires_in is absent. Fallback to the documented ~6h TTL + a structlog warning when the JWT exp is unreadable, so a fresh token is treated as live instead of stale. * [F093] serialize concurrent live-chat spawns under a per-agent lock The intake and secretary agent ids are each a single fixed id, so two concurrent start_intake_session / start_secretary_session calls raced on the container name (docker run --name roboco-agent-) and the _instances[] write: both passed the reap-prior check before either registered, both ran docker run, and the last _instances write won, orphaning the other container + its relay. Add _intake_spawn_lock / _secretary_spawn_lock (asyncio.Lock) and wrap the _spawn_intake_container / _spawn_secretary_container bodies so the second start waits for the first to fully register before its own reap-prior check runs. Distinct from self._lock (which stop_agent takes) to avoid a reentrancy deadlock: the spawn body holds the spawn lock then calls stop_agent (acquires self._lock) — lock order is always spawn_lock -> self._lock, never the reverse. * [F094] add a persistent-probe-failure escape hatch to provider parking _on_probe_failure only incremented the failure counter and, at 10 failures, sent a one-shot CEO notification. It never cleared the tracker, never gave up, never fell back to time-expiry. _do_probe returns False for any non-2xx AND any httpx error, so a permanently unreachable probe endpoint (removed API key, network partition to the probe host, misconfigured base URL) kept the provider parked forever — every agent on it gated by _provider_spawn_parked, their tasks reaped to pending but the spawn gate queuing every spawn, sitting pending forever. The only recovery was the operator manually clearing the Redis key. Past _PROBE_GIVE_UP_THRESHOLD (30) persistent failures, fall back to the same time-expiry optimism the unprobeable-provider path uses (_do_probe returns True when there is no probe URL): clear the park and resume parked agents. If the provider is genuinely still down the real workload attempts re-park via the 429/5xx path, so this is bounded burn — strictly better than a silent forever-strand. Kept above the CEO-notify threshold (10) so the operator still gets the notification first. * [F095] orchestrator: parked-provider spawn short-circuits before expensive prepare spawn_agent ran the full _prepare_agent_spawn (writes blueprint/settings/ briefing/MCP files, ensures the image, registers a STARTING instance) every dispatcher tick only to bail at the after-prepare parked-provider check — wasting all that file I/O while the provider stayed parked and leaving a STARTING instance registered then downgraded to OFFLINE. Move the parked check before _prepare_agent_spawn: resolve the route cheaply via _resolve_agent_route (only provider_type is needed) and bail with a minimal unregistered OFFLINE instance. The existing-running check stays first (inside the lock) so a live agent is never replaced; a TOCTOU re-check guards the unlocked window before prepare; the after-prepare check is kept as a rare-race defense (a park landing during prepare). * [F096] orchestrator: serialize fire-and-forget respawn persists per commit order _persist_respawn_record is fire-and-forget per gate mutation; a respawn loop fires count 1->2->3->4 in quick succession, scheduling one persist per increment for the same (agent_slug, task_id). The ON CONFLICT DO UPDATE upsert is row-level race-free, but the fire-and-forget tasks can still COMMIT out of order: a slow stale persist (count=2) scheduled first can resolve AFTER a fast fresh one (count=4) scheduled second, leaving the durable row at the stale low count and re-burning the strike threshold on restart. Fix: acquire self._respawn_persist_lock (new asyncio.Lock) as the FIRST await in _persist_respawn_record, so acquisition order = task creation order (FIFO ready queue) = logical schedule order, and commits land in that order. The durable row always ends at the latest logical value. The lock lives in the bg task, so the dispatcher hot path never blocks; persists are best-effort and a slow one queuing the rest just delays the durable catch-up (in-memory record stays authoritative). * [F097] orchestrator: back off grok re-park retry_after within a rate-limit episode _probe_target returns (None, {}) for grok — the grok CLI's xAI endpoint is closed and the SuperGrok OIDC access token is not a valid bearer for the metered api.x.ai, so a real probe would either no-op or strand grok parked forever. _do_probe treats url-is-None as success (time-expiry optimism), so once the 60s retry_after passes the probe loop optimistically clears the grok park, a cleared park dispatches a fresh grok agent that hits the still-active xAI 429, exits 75, and re-parks — a flat ~90s crash-retry cycle for the whole xAI rate-limit window (each cycle costs container startup + a rejected grok call). Fix: track _grok_repark_count + _grok_last_park_at in _park_grok_rate_limited and back the re-park retry_after off exponentially within one episode (60 -> 120 -> 240 -> ... capped at 2**4 = ~16min cycle) so the churn dampens. A gap past _GROK_REPARK_EPISODE_GAP_S (25min, > the capped cycle) means no re-park for that long => the rate limit actually lifted => a fresh episode resets the count to the base 60s, so recovery latency isn't penalized across episodes. The first park in a fresh episode is unchanged at 60s. * [F098] orchestrator: keep waiting record through a re-park during probe-success resume resolve_wait deleted the waiting record (in-memory + durable) BEFORE calling spawn_agent. A re-park in the window between the probe-success clear and the spawn — the provider's rate limit lifts then immediately re-limits, or a second provider limit lands — bails spawn with an OFFLINE instance (the parked-provider short-circuit). Deleting the record first orphaned the agent: with no record the probe-resume loop can never revive it and the spawn gate bails every tick, so the agent is lost until the operator intervenes. Fix: spawn first, then tear down the record only once a container actually launched (instance.state == ACTIVE). On an OFFLINE bail the record stays so the next probe-success re-attempts the resume. On a spawn EXCEPTION the record is torn down + re-raised so the probe loop doesn't keep re-resuming a task that moved to a different state (e.g. readiness refused -> task auto-blocked) — matching the pre-fix behavior where the record was deleted before the spawn. * [F099] wire pr_pass/pr_fail self_review block in the spec gate The pr_pass/pr_fail ActionSpecs carry self_review_block=True, but _gate_preflight never populated Context.original_developer_slug, and actor_slug was read off agent.slug — which GatewayAgentView does not carry, so it was always None in production. The block was structurally dormant: a reviewer who was also the original developer of the assembled PR could pass (or fail) their own work. The service-layer _validate_not_self_review backstop only covers qa/documenter, not pr_reviewer, so the spec gate is the only defense. Set actor_slug=str(reviewer_agent_id) (GatewayAgentView has no slug, so the UUID is the identity) and original_developer_slug from the original_developer marker (a UUID stored as a string). Both resolve to UUID strings, so the spec's string-equality comparison fires when the reviewer IS the recorded original developer. The marker is never set on assembled coordination tasks (only on dev-leaf tasks at QA/doc claim), so the block stays dormant by design in production — but the gate is now correctly wired to fire if the marker were ever set to the reviewer. Zero production behavior change; the dormant-in-production state is pinned by the no-marker test. * [F100] atomic Redis probe-failure counter via server-side Lua increment_probe_failures / reset_probe_failures did a non-atomic get_state (GET) -> mutate -> set (SET) in Python. A concurrent activate() re-park writes a FRESH episode blob (probe_failures: 0 + fresh activated_at / retry_after / affected_agents / kind); if the stale increment's SET landed after the fresh activate's SET, the stale blob overwrote the fresh episode metadata AND un-reset the counter (clobbering the new episode). Redis single-threads a Lua EVAL, so a server-side read-modify-write is indivisible: activate's SET is serialized entirely before or after the script, never interleaved between the script's GET and SET. The two scripts mutate ONLY probe_failures, so every other episode field survives the bump. activate stays a single atomic SET (a fresh episode resetting the counter to 0 is correct semantics). * [F101] enforce PR-open state gate on gateway open_pr (parity with HTTP path) * [F102] make project_id mandatory on pr_target (close cross-repo pr_number collision) * [F103] make project_id mandatory on close_pull_request (close cross-repo collision) * [F104] fail-closed on conventions resolution errors (block gate no longer silently disabled) * [F106] compound (timestamp, id) keyset cursor for message pagination get_messages used strict timestamp inequalities with a non-deterministic order_by(timestamp.desc()), so equal-timestamp messages were cut by limit on one page and excluded (strict < T / > T) from the next — they vanished across pages. Bundled the (timestamp, id) pair into a MessageCursor dataclass so the next page resumes exactly past the cursor's id at the shared timestamp (or_: strictly-older OR same-timestamp-smaller-id for before; the mirror for after), with a deterministic order_by(timestamp.desc(), id.desc()) so the last-item cursor is unambiguous. id is None for a legacy timestamp- only cursor (strict inequality, prior behavior). The route builds cursors from the flat before/before_id + after/after_id HTTP params; the schema now carries the tie-breaker ids. Also clears PLR0913 (cursors replace the before_id/after_id params). * [F107] defer Redis bus publish until DB commit (no phantom notifications) deliver() and _persist_and_deliver() ran inside the caller's open transaction: the notification row was flushed but not committed, yet NOTIFICATION_SENT was published to the Redis bus immediately. A commit failure (DB hiccup, constraint, asyncpg error) rolled the row back while connected WebSocket clients had already received a push for an id that no longer existed — a phantom notification (notify_get -> NotFoundError). Added a deferred-publish (transactional-outbox) helper: defer_bus_publish enqueues the event on session.info and registers one-shot after_commit / after_rollback listeners on session.sync_session the first time it is called for that session. On commit, the after_commit listener schedules the async drain via asyncio.create_task on the running loop (the listener fires synchronously inside await AsyncSession.commit, so the loop is active); the task handles are stashed on the session so callers/tests can await them. On rollback, after_rollback drops the pending queue — a rolled-back txn emits nothing. deliver() now builds the per-recipient events up front (data materialized to strings, so deferral is safe even if the ORM object later expires) and defers each; the delivered_at DB marker stays in-tx (rolls back with the row). The bus block stays best-effort (try/except + log) so a bus-init failure never propagates or rolls back the notification row — matching the prior inline semantics. This fixes every deliver/_persist_and_deliver caller at once (the two cited in F107 plus the orchestrator + task.py deliver sites), since they all commit the session afterward (the deferred publish fires on that commit; the row is durable by the time the event goes out). * [F108] atomic replace_chunks: single-txn delete+insert closes reindex race * [F109] playbook curation status guards: approve/reject draft-only, archive approved-only * [F110] draft slug TOCTOU: catch IntegrityError on flush -> ConflictError (no 500) * [F113] collapse WorkSession creation to the validated service path _create_work_session_if_needed constructed WorkSessionTable directly, duplicating WorkSessionService.create's validation (existing-active check, single-active-per-task supersede, project/task existence). The two sites had drifted. Route through WorkSessionService.create instead, mapping ConflictError to the idempotent 'if needed' None. Remove the now-dead _supersede_other_active_sessions (create's supersede_active_sessions_for_task replaces it). Fix three pre-existing RED tests surfaced by the sweep (all confirmed failing on the F110 commit before this change): - test_fail_qa_work_session_fallback_excludes_qa_session: inserted two ACTIVE work_sessions per task, violating uq_work_sessions_one_active _per_task (migration 047). The QA session is now ABANDONED — still in the fallback query's result set (the query filters by task_id + agent_id, not status), so the exclude filter (agent_id != qa_id) is still exercised and the dev is resolved. - test_ceo_reject_routes_coordination_task_to_main_pm / test_ceo_reject_routes_batch_umbrella_to_main_pm: ceo_reject emits an audit row keyed to CEO_AGENT_ID, but the tests never seeded the CEO agent row (fk_audit_log_agent_id_agents). Seed the CEO agent (get-or- create, mirroring test_ceo_reject_writes_handoff_journal). * [F114] single-claimant guard on pr_gate_claim pr_gate_claim delegated straight to _qa_or_doc_claim, which overwrites claimed_by / active_claimant_id with no single-claimant check. Two reviewers race-claiming the same awaiting_pr_review task would last-write-wins overwrite the first claim, and the first reviewer's subsequent pr_pass / pr_fail would actor-mismatch against the new owner (wasting a review cycle). The orchestrator's gate dispatcher already prevents double-reviewer-dispatch in normal flow (one task -> one team -> one reviewer + is_agent_active + per-tick spawned set), so the race is only reachable via direct concurrent API calls (defense-in-depth). Add a role-aware single-claimant guard in pr_gate_claim: lock the row FOR UPDATE (serialize concurrent claims, mirroring the dev claim path), then refuse only when the task is already actively claimed by a DIFFERENT PR-reviewer. The gate task is owned by the PM at entry (submit_for_review does not clear ownership, unlike submit_for_qa), so the guard must distinguish a PM/dev owner — which the first reviewer legitimately overclaims — from a competing reviewer claim; checking the existing claimant's role (pr_reviewer) does exactly that. A re-claim by the same reviewer is idempotent (skipped by the != check). The gateway claim_gate_review handler already maps a None return to a clean invalid_state envelope ('it may already be claimed; give_me_work for the next'), so no gateway change is needed. TDD: 3 integration tests in test_task_service_basics.py — reject a second reviewer race-claim (returns None, first claim intact), allow the first reviewer when the PM owns the root (regression guard for the PM-owns-at-entry model), idempotent re-claim by the same reviewer. Confirmed the reject test RED first (race-claim succeeded, overwriting reviewer1). * [F115] sample monorepo per (repo,workflow)/(repo,command) not per repo The CI-watch and dep-update loaders collapsed a monorepo's cell-projects to one canonical entry per repo (slug-sorted-first), so a repo whose cells each carry their OWN ci_watch_workflow / dep_update_command had only the canonical cell's workflow/command sampled — a red on another cell's workflow or drift on another cell's lockfile was missed (under-count). Refactor the shared one-per-repo collapse into _projects_one_per_key, keyed by repo identity for external-PR discovery (unchanged: one review per PR per repo), by (repo, effective workflow) for CI-watch, and by (repo, command) for dep-update. Each distinct workflow/command is now sampled once; the engines' per-git_url fix-task dedup still prevents duplicate fix tasks for the same repo. _projects_one_per_repo now delegates to _projects_one_per_key. key_fn uses a string annotation (Callable lives under TYPE_CHECKING, like the existing Coroutine/Iterable annotations at lines 4193/5279). * [R115] originate ci_watch/dep_update fix tasks as PLANNING coordination roots The Main-PM-code-impossibility guard (commit e202ce39, Thread 4 of this audit) made team=MAIN_PM + task_type=CODE impossible — a Main PM coordinates, it does not write code. But the ci_watch and dep_update engines still originated their fix tasks as task_type=TaskType.CODE assigned to main-pm, so task_svc.create raised MAIN_PM_NO_CODE and NO fix task was ever opened — a regression introduced by the earlier audit fix (confirmed: the engine tests pass at e202ce39~1 and fail at HEAD). Mirror the hardened self_heal_engine precedent (self_heal_engine.py:197) which already uses task_type=TaskType.PLANNING for its Main-PM coordination root with an explicit 'decompose the fix and delegate the code work to a cell dev — the Main PM does not write the fix itself' description. Both engines now originate PLANNING coordination roots with matching delegation guidance in the description + acceptance criteria. confirmed_by_human stays True for both (they ride the normal delivery flow without the CEO gate, unlike self-heal — intentional per the architecture). The dedupe/open-cap queries (list_open_ci_watch_tasks / list_open_dep_update_tasks) key on source + non-terminal status + git_url, NOT task_type, so the type change does not break dedup (still one open fix task per repo). The two source-test fixtures (test_ci_watch_source / test_dep_update_source) created CODE+MAIN_PM tasks directly to exercise the listing queries — same guard violation; switched to PLANNING (the queries assert on source/status, not task_type, so the fixture type matches the engines' corrected type). * [F116] hold the read-clone lock across the dep-probe local clone dry_upgrade_changes_lockfile called ensure_read_clone (which syncs the read clone under the _meta-conventions lock then releases it) and ran 'git clone --local --no-hardlinks ' OUTSIDE the lock. A concurrent ensure_read_clone -> _sync_read_clone (fetch + hard-reset to origin's default branch) could mutate the read clone's working tree / object db mid-clone, racing the clone and producing an inconsistent or failing probe. Split _probe_lockfile_change into _clone_local_into (the local clone, run under the read-clone lock) + _probe_lockfile_on_clone (the upgrade + git status, run without the lock on the now-independent copy). The probe acquires _ensure_lock_for(slug, '_meta-conventions') — the same lock ensure_read_clone syncs under — and holds it only for the clone step; the upgrade operates on the full --no-hardlinks copy and never touches the read clone, so the lock is released before it to avoid blocking conventions reads for the upgrade duration. The tiny gap between ensure_read_clone releasing the lock and the probe re-acquiring it is safe: any concurrent _sync_read_clone completes under the lock before the probe acquires, so the clone reads a stable state. * [F117] stop the orchestrator in lifespan shutdown BEFORE closing the DB The lifespan shutdown closed OptimalService + the DB, and only THEN did bootstrap's finally block call orchestrator.stop() — so stop() ran with the DB already closed. stop() drains fire-and-forget _bg_tasks writes (respawn_tracker upserts, audit-log rows) and stop_agent finalizes work sessions / agent state, all needing the DB still open; closing it first silently dropped those final writes (the durable PM-respawn counter's last few strikes, the metrics-bearing audit trail tail). Move orchestrator.stop() into the lifespan shutdown path, BEFORE close_optimal_service + close_db, guarded by a new get_orchestrator_or_none() safe accessor (no crash when no orchestrator is wired — tests, skip_orchestrator). bootstrap's finally-block stop() becomes an idempotent safety net: stop() gains a _stopped flag (getattr-guarded so __new__- constructed test instances still stop) so the double-call is a clean no-op, not a re-stop of already-stopped agents / re-drain of an empty bg set. * [F118] coerce a lone-string where_to_look into a list where_to_look is a list-typed handoff field like consequences/next_steps but was the only one NOT in the _wrap_scalar_in_list field_validator. A well-intentioned where_to_look='src/api/' 422'd at the route with no remediation envelope, and the agent's retry loop tripped the do-server circuit breaker — the exact failure mode the other list fields were hardened against. Add it to the mode='before' validator so a lone string is wrapped into a one-element list before type coercion. * [F119] sender reaps dead sockets on send error instead of waiting for receive idle timeout * [F120] release a stopped agent's claimed task immediately on budget-kill/shutdown * [F122] name the already-open PR in submit_up's None-state remediate submit_up's create_pr pre-side-effect opens the cell→root PR BEFORE submit_for_review runs (its pr_created gate requires it — lifecycle.py:1338-1343). When submit_for_review returns None (a concurrent state change raced the task out of in_progress between the precondition gate and the composed action), the old remediate ('check task state — must be in_progress with PR ready') hid that the PR was already open on GitHub — an orphaned external artifact the PM could not reconcile. Mirror submit_root's F016 None-envelope remediate: name the open PR, point the PM at re-fetch + reconcile, and note create_pr is idempotent so a re-issue re-attaches to the existing PR (no duplicate). Pure message improvement — zero behavior change; reordering is off the table (create_pr must precede the pr_created gate). * [F124] re-check dependency state before releasing a dependency-blocked claim The unmet_dependency guard read dependency state via an unlocked SELECT, then fired release_dependency_blocked_claim (a state mutation: claimed/in_progress -> pending, clears branch_name, abandons WorkSession) as a side-effect BEFORE returning the rejection. An upstream dependency that reached a terminal state (completed/cancelled) in the microseconds between the read and the release left the task NEEDLESSLY released — its branch cleared + WorkSession abandoned + assignee bounced, only to be re-dispatched + re-claimed when the dependency- completion re-dispatch fired a moment later. Re-check unmet_dependency_ids immediately before the release and skip it (returning None — proceed) when the upstream just completed. Dependencies are monotonic (unmet -> met only; terminal states never reopen), so a fresh read that now finds them met stays met: safe to proceed without releasing. The 'still unmet' path is byte-for-byte the prior behavior (no regression). The cross-task residual window (upstream completes between the re-check and the release) is not closable by a row lock on the dependent, but the re-check narrows the window from [first read -> release] to [re-check -> release], and in the common case the first read already sees met (no guard fires). No committed-work loss either way (a dependency-blocked task has none; the branch ref + commits persist across the branch_name clear). * [F125] serialize same-parent delegate via per-parent advisory lock The delegate sibling-dedup guard read the parent's existing subtasks via an unlocked get_subtasks SELECT (the dedup read) then created the subtask (the write) with no DB serialization between them. Two concurrent delegate calls for the same parent (PM re-delegating while a reaper re-dispatches, or two orchestrator ticks racing) each read a duplicate-free sibling set, each passed the dedup guard, and each created a subtask — the parent got the duplicate the guard exists to prevent (the smoke-run runaway pattern). Fix: a PostgreSQL transaction-scoped advisory lock keyed by the parent task id (seed 1, disjoint from the per-agent claim lock's seed 0), acquired at the top of the delegate body before the first get_subtasks read (the briefing context read AND the dedup sibling read) and held through create_subtask's flush + the outer request commit. The second concurrent same-parent delegate blocks until the first commits, then its dedup read sees the committed sibling and is rejected. Per-PARENT (not per-agent): a coordinator PM legitimately delegates many subtasks under one parent in quick succession and plans many roots in parallel — a per-agent lock would serialize all of a PM's delegates and regress the PM coordinator concurrency feature. The per-parent lock serializes only same-parent delegates (the dedup invariant is per-parent) and leaves different parents untouched. TDD: red-first ordering test (lock acquired before first get_subtasks read and before create_subtask) + no-regression test (create still runs). * [F127] per-task advisory lock prevents open_pr milestone double-emit open_pr's idempotent re-entry guard (pr_number is not None) read t.pr_number from an unlocked fetch. Two concurrent same-task open_pr calls (the alive-but-unresponsive respawn race) both fetched pr_number=None, both passed the guard, both ran the runner (GitHub 422 ensures one PR), and both reached _record_milestone_progress -> a double-emitted 70% 'opened PR #N' entry. Fix: acquire_task_lock (pg_advisory_xact_lock, seed 2) before the fetch, held through the runner + milestone + request commit. The second concurrent call blocks until the first commits, then its fetch sees the committed pr_number and the idempotent guard short-circuits without re-emitting. Per-task (single- active-task guard means same-task concurrent open_pr is only the bug case). * [F128] require active claim on explicit-task content posts _verify_explicit_task_ownership checked assigned_to, which is stale across a reap/handoff (persists until reassignment; active_claimant_id is cleared on release). A reaped agent could keep posting say/dm/note to its former task. Add the active-claimant check when assigned_to == caller; assigned_to=None keep its existing allow (read-side inspection between reassignments uses evidence, which has its own ownership path). Existing 'active owner' test mocks passed assigned_to=agent_id without active_claimant_id; production sets both together on claim, so the mocks were incomplete. Updated to set both — realistic, not a behavior change. * [F129,F130] harden quality gate _run_one exit status + timeout cleanup F129: _run_one returned 'proc.returncode or 0', masking a None returncode (communicate returned without a recorded exit code — process killed out-of-band) as 0 / success. Treat None as a non-zero failure (fail-closed). F130: on timeout, _run_one killed the subprocess but never awaited wait() — communicate() was cancelled so it never closed the stdout/stderr pipes, leaving a transient zombie + leaked FDs. Await wait() after kill() to reap the process and close the transports. * [F132] timeout the conventions validator + reap on hang _run_conventions_validator awaited proc.communicate() with no timeout — a hung subprocess (tree-sitter deadlock, huge repo) hung the i_am_done/pr_pass gate forever and orphaned the python subprocess on orchestrator restart. Wrap communicate() in wait_for(120s); on timeout kill+wait the proc and fail closed (could_not_run=True → block gate refuses the submit), matching the validator's own fail-loud philosophy. * [F135] re-check activity before sweeper closes a session (TOCTOU) sweep_timed_out_sessions read last_activity_at once at the candidate SELECT, then closed. A message landing in that window refreshed last_activity_at in the DB, but the sweeper closed on its stale in-memory value — closing a just-used session. Re-read last_activity_at fresh right before the close and skip if the session is no longer timed out. * [F136] cancel startup indexing task on OptimalService.close() close() cancelled only the periodic update task, then cleared the plugins. The startup _indexing_task (background auto-index, slow Ollama / large repo) could still be mid-flight at shutdown and write against closed/cleared plugins. Cancel and await _indexing_task FIRST (its tail starts the periodic task, so ordering also prevents a late periodic spawn), then the periodic task, then clear plugins. * [F139] scope active_task_owns_branch to the polled project active_task_owns_branch did an unscoped WHERE branch_name = ? — a cross-project branch_name collision (UUID-derived 8-char prefixes, theoretical) made the internal-PR reviewer skip the WRONG project's PR (project A's leftover PR skipped because project B happened to have an active task with the same branch). Pass project_id (in scope at the orchestrator call site) and add TaskTable.project_id == project_id to the WHERE. Correct for single-project tasks and MegaTask multi-repo batches alike: each root-subtask carries its own project_id matching its own repo, so a branch on project A's repo is owned only by a task whose project_id == A. * [sweep] strip Fxxx audit-ID tokens + trim bloated comments/docstrings + add behavior-change docs Post-audit sweep over the 135 audit-fix commits since 19a474d3: 1. Stripped every # Fxxx: audit-ID token from comments AND every Fxxx token from docstring openings across 211 blocks / ~626 lines. The CEO flagged these twice: audit-issue IDs in code confuse future devs/agents. The descriptive text is preserved; only the Fxxx token is removed (and bloated narrative blocks trimmed to 1-3 lines keeping the one non-obvious invariant). 2. Trimmed bloated comments/docstrings to the concise standard (1-3 lines). 3. Added missing behavior-change docs for the audit-fix batch: prompts/roles (documenter, pr_reviewer, qa), user-facing docs (api auth, websockets, agent-gateway, megatask, merge-model, task-lifecycle, grok, resilience, conventions, panel, security, troubleshooting), and the RAG corpus (cell-pm, main-pm, pr-reviewer, qa roles; conventions; messaging-tools; escalation; megatask; task-claiming workflows). Comment/docstring/prose ONLY — zero code-line edits (verified: the diff contains no def/class/return/if/for/await/assignment/call lines). Gates green: ruff format + ruff check clean, mypy clean on roboco/. The only pytest failures are the pre-existing sync_branch tracing-decision gap (B1, 250be5c2) — not sweep-caused and tracked separately. * [fix] register sync_branch in VERBS_WITHOUT_TRACING sync_branch (B1, 250be5c2) is a git-only rebase+force-push verb (composes=(), no DB transition, side_effects=()) but was never registered in the tracing parity tables, so test_every_intent_verb_has_a_tracing_decision failed. Mirrors open_pr: a mechanical git op with inline preconditions (ownership), no journal/plan rationale required. * chore(release): 0.14.0 * [fix] resolve 16 mypy errors across 9 test files (make quality gate) type-clean the test files so make quality (mypy roboco/ tests/) is green: - Any-typed locals for the two TypeError-asserting scoping tests (bypass the required-arg check without getattr/ruff B009) - Any-typed view for the shutdown-drain _drain_bg_tasks override (bypass mypy method-assign without setattr/ruff B010) - cast("uuid.UUID", ...) / cast("UUID", ...) for SQLAlchemy UUID[Any] returns (TC006-quoted), config=None for AgentInstance stubs, None-narrowed await_args, Iterator return on a yielding fixture, UUID annotation on the _task helper. No type:ignore / noqa. * [docs] regenerate lifecycle artifacts for sync_branch + branch-keyed submit_root gate The committed artifacts were stale: lifecycle.py grew the sync_branch verb (B1) and the branch-keyed submit_root gate description (B2/B3) but the generated markdown/json were never regenerated. make foundation-check enforces artifact==generator(lifecycle.py); regenerating restores that. No source change — pure generator output. * [refactor] reduce xenon C-rank blocks to A (behavior-preserving) Extract helpers / flatten conditionals in 11 blocks that rated C(11)+ under xenon --max-absolute B, dropping pr_gate.py module rank B->A in the process. Pure move-and-call refactors: each extracted helper holds the original logic verbatim and the caller delegates to it; no control flow, return values, or side effects changed. Sites: validators._extract_strs, sequencing.dev_task_collision_edges, evidence_builder.build_task_handoff, intake_driver._coerce_draft, task.claim_task_for_agent (2 guards), prompter.create_task_from_draft (validate+assignee), pr_gate._gate_decision (3 helpers), orchestrator._handle_stopped_container + _reap_with_service, _impl._create_subtask_from_inputs + complete. _impl helper returns tuple[TaskNature, list[str]] to preserve mypy narrowing of acceptance_criteria at the TaskCreateRequest site. Also fix vulture: rename unused __aexit__ param tb->_tb in test_conventions_cache_put.py (was hidden while xenon short-circuited the gate). * [security] bash-guard uv run --active deny + CodeQL path-traversal fixes Fix 1 (be-dev-1 brick prevention): bash-guard now denies 'uv run --active' and 'uv run'/'uvx' against /app targets. In the agent container VIRTUAL_ENV=/app/.venv is baked globally, so 'uv run --active' always resolves onto the image-baked MCP-gateway venv and uv rebuilds it, deleting /app/.venv/bin and bricking every MCP server spawn. Bare 'uv run' (workspace .venv, cwd-relative) is untouched. CodeQL fixes: - docs.py: replace bypassable '..' substring guard with a resolve-and-contain helper (_resolve_contained_path). An absolute path made pathlib reset (base / '/etc/passwd' == '/etc/passwd'), letting read_doc/delete_doc reach arbitrary files. Applied to both sinks. - orchestrator.py: _safe_agent_path_segment at the spawn_agent chokepoint (rejects traversal-shaped agent_id before any fs op) and inside _remove_container (slug guard before the log-dir mkdir, defense-in-depth). - agent_sdk/server.py: /usage/sync transcript_path now resolved and contained under ROBOCO_TRANSCRIPT_DIR with a .jsonl suffix requirement (was Path(raw) — unauthenticated endpoint could stat arbitrary files). TDD RED->GREEN across all four; make quality green (4890 passed). * [fix] enum-parity gate: drop false-green mask, skip empty/unmigrated DB The foundation-check gate ran the enum verifier behind `|| echo "(skipped — postgres unreachable)"`, which swallows ANY non-zero exit — including real drift — and prints 'All quality gates passed'. On a host with a dockerized but empty/unmigrated `roboco` DB (0 tables: the agentrole/team enum types don't exist), the verifier connected, found every foundation value 'missing', exited 1, and the mask relabeled it 'skipped' → false-green. Fix: - scripts/verify_postgres_enums.py: move skip semantics INTO the script. Distinguish unreachable (skip, exit 0), DB-not-migrated/both-enum-types- absent (skip, exit 0), real drift (exit 1), match (exit 0). Extract pure enum_drift + should_skip_for_unmigrated helpers + a type_exists probe so an empty DB is 'no migrated target', not drift. - Makefile: drop the `|| echo` mask — real drift now fails the gate. TDD RED->GREEN (10 tests); make quality green (10906 passed). * [security] docs path guard: reject '.'/empty segments for clean 400 _resolve_contained_path used an '..' substring ban, which (a) left rel='.' passing the guard — read_doc/delete_doc then got the base DIRECTORY itself and raised IsADirectoryError (500) instead of a clean ValidationError, and (b) false-rejected legit filenames containing '..' like 'v1..v2.md'. Replace the substring ban with a raw-segment check (rel.split('/')) that rejects any '.', '..', or empty segment. Path(rel).parts was the wrong tool — pathlib collapses '.' and empty segments on 3.13, hiding them. The split check catches '.' / 'a/./b' / 'a//b' / '..' / 'a/../b' while allowing 'v1..v2.md' ('..' inside a filename, no bad segment). The post-resolve parents-containment check (the real defense) is unchanged. TDD RED->GREEN (4 new tests); make quality green (10910 passed). Follow-up to the CodeQL path-traversal review: the two CodeQL 'High' alerts on this guard are false-positives-on-the-fix (resolve-and-contain already contains the bypass); this hardening closes the one genuine low residual (rel='.' -> 500) the review surfaced, which CodeQL did not flag. * [F123] per-task git worktrees — coordinator PM roots no longer clobber each other A coordinator PM (Main/Cell) legitimately holds several in_progress roots at once, but its clone is one 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 discarded "abandoned cruft from a finished task" — neither root was finished — and the mutation was non-transactional with the DB claim (rollback restored DB fields, not the tree). Each task now gets its own working tree via `git worktree add` under `{clone_root}/.worktrees/{task-short}/` on the same clone; the F123 reset dissolves (a fresh worktree is clean by construction). The shared clone keeps the real `.git` store, the per-project `.venv`, and `.uv-python`; each worktree gets a `.venv -> ../../.venv` symlink so uv resolves the shared venv (no per-worktree re-sync), and `.uv-python/` is now gitignored. Branch-by-name ops (push/pull/fetch/pr_merge/diff) run from the clone root; checkout/HEAD-moving ops (create_branch/commit/rebase/checkout) target the worktree. Spawn resolves the worktree from current_task_id each spawn and `-w`'s it; resume re-attaches a pruned worktree before launch; claim-rollback force-removes on mid-claim failure; terminal cancel removes the worktree (the stale-claim reaper does not — it routes to pending for a re-claim that reuses it). Routing-gap followup (the switch missed two checkout-dependent ops that still resolved the clone root — both deploy-blockers): - rebase_onto_base does `checkout ` + `reset --hard origin/` cwd= workspace; post-worktree the branch is checked out in the linked worktree, so a checkout in the clone root is refused ("already checked out"). sync_task_branch + rebase_pr_for_task now resolve the worktree via _worktree_for_task and rebase there (checkout becomes a no-op on the already-checked-out branch). The destructive reset --hard is pre-existing semantics, preserved. - conventions_check_for_task ran the validator with --root ; it reads (root/rel).read_bytes() → default-branch content, not the dev's worktree changes (newly-added files false-pass, conventions block gate silently disabled). Now resolves the worktree and runs the validator there, so i_am_done/pr_pass gate against the real diff. Invariants untouched: only the CEO merges master (no merge/release path touched); /app/.venv (image-baked MCP-gateway venv) stays sacred; coordinator-PM concurrency exemption unchanged — only the workspace resolution underneath became per-task. 9085 pass / 0 fail (2272 skipped: local Docker down → test PG unavailable; 95% with-PG on NAS). ruff/mypy(xenon/radon/vulture/bandit/pip-audit/deptry/alembic/ import-linter/foundation-check) green. * [docs] CHANGELOG: F123 per-task worktrees + routing-gap followup * [F-fix] preserve respawn strike count across a restart status mismatch restore_respawn_tracker re-stamped last_check to the restore time (F034) but left last_status at the PRE-RESTART persisted value. On the first post-restart spawn, _pm_respawn_should_gate compares record["last_status"] to the live status; a mismatch (a reaper/external transition in the restart gap, not evidence the wedge cleared) reset count to 1 and disarmed the breaker — re-burning the whole strike threshold against a still-wedged task, exactly the re-burn the respawn_tracker table (migration 051) was built to prevent. Re-stamp last_status to the LIVE status at restore (mirrors the last_check re-stamp). The live progress heuristic in _pm_respawn_should_gate (status advanced between spawns => reset) is untouched; only the restore path changes. Conservative: if the wedge genuinely cleared during the gap, the breaker may false-trip on the first spawn — the safe direction (pause + alert the CEO once, never destroy state) rather than silently re-burning the budget. * [F-fix] gate the worktree .venv symlink on the clone-root venv existing _link_shared_venv created worktree/.venv -> ../../.venv unconditionally, so when the clone-root venv was not yet provisioned (the near-zero gap before install_dev_deps completes) the symlink dangled — uv then errored or re-synced a worktree-local venv that the lexists guard could not later replace, silently breaking the shared-venv optimization for that worktree. Only create the symlink once clone_root/.venv exists; a later ensure (the resume/commit path re-runs ensure_workspace -> install_dev_deps) self-heals the link. No destructive replacement of an existing real dir at worktree/.venv (recovery of an already-clobbered worktree venv is out of scope). * [F-fix] ensure_worktree_before_spawn: release claim + abort on fatal ensure failure A bare except swallowed every failure, so a pruned worktree whose branch ref was also gone launched the container at a missing -w path. A fatal WorkspaceError (branch ref gone, worktree unrecoverable) now releases the claim so the next claim rebuilds via create_branch and aborts the spawn; a transient failure aborts without releasing (next tick retries). The swallow also masked the happy-path tests' non-awaitable mock. * [F-fix] complete/ceo_approve: remove per-task worktree on terminal completion Only cancel() + create_branch rollback removed per-task worktrees, so completed/merged tasks leaked their worktree on disk until the whole agent/project was deleted. Both terminal->completed paths (cell-PM complete after leaf PR merge; CEO ceo_approve after root->master merge) now remove the assignee worktree best-effort. Terminal-only (a dev task bounces needs_revision and needs its worktree back); no-op for branchless; best-effort so a removal failure never blocks completion. Reaper reuse-on-reclaim unchanged. * [F-fix] worktree-cleanup tests: preset execute on local session mock Assigning svc.session.execute tripped mypy method-assign (session is typed as a real AsyncSession). Build the session as a local MagicMock, preset execute on it, then hand to TaskService — the proven _service_with idiom. * [feature] loop-prone notification re-fire guard (sweep #4) TASK_ASSIGNMENT/REVIEW_REQUEST/DOCUMENTATION_REQUEST/BROADCAST re-fired by a coordinator PM every tick while a task sits in a state, flooding inboxes. The DB purpose-dedup never fires for these (ACK_REQUIRED_BY_TYPE marks them requires_ack=False) and _persist_and_deliver had no dedup at all. New roboco/services/notification_dedup.all_recipients_recently_notified: a 60s Redis SET NX window per (type, sender, recipient, task). First fire acquires keys for fresh recipients; a later fire in the window is suppressed when no recipient was fresh, converging the storm. Fail-open (Redis down -> never suppress). One-shot types bypass entirely. Wired into both creation chokepoints: NotificationService._create_notification (after recipients resolved, before the DB dedup block) and NotificationDeliveryService._persist_and_deliver (before session.add). TDD: helper + both chokepoints, 15 new tests; SA UUID column type-leak peeled with cast at the delivery path. * [chore] sequencing: chain undeclared-surface same-assignee dev siblings + trim re-fire guard comments wire_sibling_collision_dag only wired collision edges for dev tasks that declared a surface; a PM delegating 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 unmerged (the out-of-order merge wedge). dev_task_collision_edges now falls back (only with zero declared-surface edges) to chaining each same-(project, assignee) lane by (priority, sequence). Same-assignee scoped so cross-dev parallel work is untouched; the edge lives in dependency_ids so it survives reassignment. Idempotent + incremental (stable sort, add_dependency dedupes). TDD: 8 unit fallback tests + 1 integration test (RED-verified by neutering the fallback). Also trims the re-fire-guard comments (no internal refs, terse) per the comment standard. * [chore] give_me_work/claim: enforce per-dev lane barrier (out-of-order start) 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 spawn path and i_am_idle, so give_me_work's pre-assigned path and a direct i_will_work_on claim bypassed it. A dev 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 (the merge wedge). give_me_work now filters pre_assigned through _pending_not_lane_held (a lane-held leaf is dropped, not offered). _run_claim_guards refuses a direct claim of a lane-held code task (invalid_state, parked back to pending via release_dependency_blocked_claim) in a new _lane_claim_guard helper. The predicate is CODE-only so coordinator PMs are inert; the claim guard is fail-closed on a lookup error. is not True keeps both paths inert under partial test mocks. TDD: 7 new tests (RED-verified) + 870 gateway/lane-queue regression green. * [chore] tests: drop a reintroduced type:ignore + fix two mypy annotations test_notification_delivery_refire.py reassigned svc.deliver with a type: ignore[method-assign], reintroducing a test suppression the project purged (f8262856). Use the cc: Any alias pattern instead. Also fix uuid4-as-a-type -> UUID in the lane barrier helper, and cast the SA UUID column leak in the sequencing integration test. mypy roboco/ tests/ clean on all touched files. --------- Co-authored-by: Renn F --- .gitignore | 2 + CHANGELOG.md | 11 + roboco/models/runtime.py | 6 + roboco/runtime/orchestrator.py | 169 +++++++++++- .../services/gateway/choreographer/_impl.py | 34 ++- roboco/services/git.py | 193 +++++++++----- roboco/services/notification.py | 20 ++ roboco/services/notification_dedup.py | 91 +++++++ roboco/services/notification_delivery.py | 23 +- roboco/services/sequencing.py | 75 ++++-- roboco/services/task.py | 92 ++++++- roboco/services/workspace.py | 175 +++++++++++++ .../test_task_service_transitions.py | 69 +++++ .../test_choreographer_lane_barrier.py | 190 ++++++++++++++ .../unit/runtime/test_respawn_persistence.py | 62 ++++- tests/unit/runtime/test_spawn_cwd_worktree.py | 230 +++++++++++++++++ .../runtime/test_spawn_worktree_ensure.py | 171 +++++++++++++ tests/unit/services/test_git.py | 132 +--------- .../unit/services/test_git_commit_worktree.py | 151 +++++++++++ .../test_git_create_branch_worktree.py | 163 ++++++++++++ .../unit/services/test_git_resolve_git_dir.py | 101 ++++++++ .../test_git_worktree_routing_gaps.py | 178 +++++++++++++ tests/unit/services/test_sequencing.py | 64 +++++ .../test_task_cancel_worktree_cleanup.py | 190 ++++++++++++++ .../test_task_claim_rollback_worktree.py | 106 ++++++++ .../test_workspace_uv_python_install_dir.py | 202 +++++++++++++++ .../test_workspace_uv_resolves_clone_venv.py | 134 ++++++++++ .../test_workspace_worktree_lifecycle.py | 242 ++++++++++++++++++ .../services/test_workspace_worktree_paths.py | 82 ++++++ .../test_worktree_cleanup_on_complete.py | 169 ++++++++++++ tests/unit/test_notification_dedup.py | 94 +++++++ tests/unit/test_notification_dedup_refire.py | 213 +++++++++++++++ .../unit/test_notification_delivery_refire.py | 78 ++++++ 33 files changed, 3673 insertions(+), 239 deletions(-) create mode 100644 roboco/services/notification_dedup.py create mode 100644 tests/unit/gateway/test_choreographer_lane_barrier.py create mode 100644 tests/unit/runtime/test_spawn_cwd_worktree.py create mode 100644 tests/unit/runtime/test_spawn_worktree_ensure.py create mode 100644 tests/unit/services/test_git_commit_worktree.py create mode 100644 tests/unit/services/test_git_create_branch_worktree.py create mode 100644 tests/unit/services/test_git_resolve_git_dir.py create mode 100644 tests/unit/services/test_git_worktree_routing_gaps.py create mode 100644 tests/unit/services/test_task_cancel_worktree_cleanup.py create mode 100644 tests/unit/services/test_task_claim_rollback_worktree.py create mode 100644 tests/unit/services/test_workspace_uv_python_install_dir.py create mode 100644 tests/unit/services/test_workspace_uv_resolves_clone_venv.py create mode 100644 tests/unit/services/test_workspace_worktree_lifecycle.py create mode 100644 tests/unit/services/test_workspace_worktree_paths.py create mode 100644 tests/unit/services/test_worktree_cleanup_on_complete.py create mode 100644 tests/unit/test_notification_dedup_refire.py create mode 100644 tests/unit/test_notification_delivery_refire.py 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()