diff --git a/docker/base-image/agent_server/routers/git.py b/docker/base-image/agent_server/routers/git.py index e114c16ea..78a41eccf 100644 --- a/docker/base-image/agent_server/routers/git.py +++ b/docker/base-image/agent_server/routers/git.py @@ -1,8 +1,10 @@ """ Git sync endpoints for GitHub bidirectional sync. """ +import json import subprocess import logging +import uuid from datetime import datetime, timezone from pathlib import Path from typing import Optional @@ -16,6 +18,17 @@ logger = logging.getLogger(__name__) router = APIRouter() +# S7 Layer 3 (#382): directory where we persist the last-observed remote +# SHA per branch. Written after every successful fetch and consumed by the +# push path as the "expected-sha" argument to `git push --force-with-lease`. +# Living under ~/.trinity keeps it out of the workspace tree while still +# surviving container restarts (the home bind mount is persistent). +LAST_REMOTE_SHA_DIR = Path.home() / ".trinity" / "last-remote-sha" + +# File the operator queue sync service reads. We append collision entries +# here when a push is rejected by the lease. +OPERATOR_QUEUE_FILE = Path.home() / ".trinity" / "operator-queue.json" + def _compute_ahead_behind(home_dir: Path, branch: str) -> tuple: """Best-effort ahead/behind counts vs origin/. @@ -84,6 +97,134 @@ def _get_pull_branch(current_branch: str, home_dir: Path) -> str: return "main" if result.returncode == 0 else current_branch +def _sha_file_for_branch(branch: str) -> Path: + """Path to the last-remote-sha file for a given branch. + + Branches can contain ``/`` (``trinity//``) so we mirror the + branch layout as nested directories. That keeps the file names readable + rather than URL-escaping the slashes. + """ + return LAST_REMOTE_SHA_DIR / branch + + +def _persist_last_remote_sha(branch: str, home_dir: Path) -> None: + """Record the remote SHA this instance observed for ``branch`` after fetch. + + S7 Layer 3: the stored value becomes the ``expected-sha`` lease on the + next push. If the remote moves out from under us (another instance + pushed in the interim) the fetch will update ``origin/`` to the + new SHA but the persisted lease is still the old one — which is exactly + what ``--force-with-lease`` is checking, so the collision is caught. + + Failure to persist is logged, never raised: it would turn a minor I/O + glitch into a hard sync failure, and the worst case is the next push + has no lease and behaves like plain `--force` (one-time regression, not + silent corruption). + """ + rev = subprocess.run( + ["git", "rev-parse", f"origin/{branch}"], + capture_output=True, + text=True, + cwd=str(home_dir), + timeout=10, + ) + if rev.returncode != 0: + logger.debug( + "No origin/%s ref yet — skipping last-remote-sha persist", branch + ) + return + + sha = rev.stdout.strip() + if not sha: + return + + try: + target = _sha_file_for_branch(branch) + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(sha + "\n") + except OSError as exc: + logger.warning( + "Could not persist last-remote-sha for %s: %s", branch, exc + ) + + +def _read_last_remote_sha(branch: str) -> str | None: + """Read the previously persisted remote SHA for ``branch``, if any.""" + path = _sha_file_for_branch(branch) + try: + return path.read_text().strip() or None + except FileNotFoundError: + return None + except OSError as exc: + logger.warning("Could not read last-remote-sha for %s: %s", branch, exc) + return None + + +def _record_push_collision(branch: str, lease_sha: str | None, stderr: str) -> None: + """Append a structured alert to ~/.trinity/operator-queue.json. + + S7 Layer 3 surfacing: when ``--force-with-lease`` rejects the push the + losing instance now knows it lost, so we write an operator-queue entry + that the backend's ``operator_queue_service`` picks up on its next + poll. The entry is an ``alert`` (no decision required) — the operator + just needs to know another instance is writing to the same branch so + they can rebind one of them. + """ + try: + OPERATOR_QUEUE_FILE.parent.mkdir(parents=True, exist_ok=True) + if OPERATOR_QUEUE_FILE.exists(): + try: + payload = json.loads(OPERATOR_QUEUE_FILE.read_text() or "{}") + except json.JSONDecodeError: + logger.warning( + "operator-queue.json is malformed; recreating before appending" + ) + payload = {} + else: + payload = {} + + payload.setdefault("$schema", "operator-queue-v1") + requests = payload.setdefault("requests", []) + + now_iso = datetime.now(timezone.utc).isoformat() + requests.append( + { + "id": f"git-collision-{uuid.uuid4().hex[:12]}", + "type": "alert", + "status": "pending", + "priority": "high", + "title": f"Git push rejected on {branch} — branch binding collision", + "question": ( + "Another Trinity instance wrote to this working branch since " + "this agent last fetched. The --force-with-lease push was " + "rejected to prevent silent data loss. Rebind one of the " + "agents to a fresh working branch before retrying." + ), + "options": None, + "context": { + "branch": branch, + "expected_sha": lease_sha, + "git_stderr": (stderr or "").strip()[:2000], + "remediation": ( + "Assign a fresh working branch to one of the colliding " + "agents (Fleet → Branch Bindings → Assign fresh branch)." + ), + }, + "created_at": now_iso, + } + ) + + OPERATOR_QUEUE_FILE.write_text(json.dumps(payload, indent=2)) + except Exception as exc: # pragma: no cover — best-effort surfacing + logger.warning("Failed to record push-collision alert: %s", exc) + + +def _is_stale_lease_rejection(stderr: str) -> bool: + """Return True if git's stderr indicates a --force-with-lease mismatch.""" + s = (stderr or "").lower() + return "stale info" in s or "stale" in s and "rejected" in s + + @router.get("/api/git/status") async def get_git_status(): """ @@ -157,6 +298,10 @@ async def get_git_status(): cwd=str(home_dir), timeout=30 ) + # S7 Layer 3 (#382): snapshot the remote SHA we just observed so + # the next push can use it as the --force-with-lease expected-sha. + if fetch_result.returncode == 0: + _persist_last_remote_sha(current_branch, home_dir) # For trinity/* working branches, compare against origin/main pull_branch = _get_pull_branch(current_branch, home_dir) @@ -297,6 +442,10 @@ async def sync_to_github(request: GitSyncRequest): cwd=str(home_dir), timeout=60 ) + # S7 Layer 3 (#382): snapshot the remote SHA for lease checks + # on the upcoming push. + if fetch_result.returncode == 0: + _persist_last_remote_sha(current_branch, home_dir) # For trinity/* working branches, pull from main pull_branch = _get_pull_branch(current_branch, home_dir) @@ -435,16 +584,53 @@ async def sync_to_github(request: GitSyncRequest): # 3. Push to remote based on strategy if strategy == "force_push": - # Force push (overwrites remote) + # S7 Layer 3 (#382): replace plain `git push --force` with + # `--force-with-lease=:`. If another instance + # wrote to the branch since we last fetched, the lease is stale + # and the push is rejected cleanly with "stale info" — rather + # than silently clobbering the peer's state (2026-04-17 + # alpaca incident). + lease_sha = _read_last_remote_sha(current_branch) + push_cmd: list[str] = ["git", "push"] + if lease_sha: + push_cmd.append(f"--force-with-lease={current_branch}:{lease_sha}") + else: + # No lease on file (first push, or we couldn't persist one). + # Use the unparameterized `--force-with-lease`, which falls + # back to remote-tracking-ref as the expected-sha — still + # safer than `--force`. + push_cmd.append("--force-with-lease") + push_cmd += ["origin", current_branch] + push_result = subprocess.run( - ["git", "push", "--force"], + push_cmd, capture_output=True, text=True, cwd=str(home_dir), - timeout=60 + timeout=60, ) if push_result.returncode != 0: - raise HTTPException(status_code=500, detail=f"Force push failed: {push_result.stderr}") + stderr = push_result.stderr or "" + if _is_stale_lease_rejection(stderr): + # Surface the collision to the operator queue. The + # losing instance now knows it lost — that's the whole + # point of the lease. + _record_push_collision(current_branch, lease_sha, stderr) + raise HTTPException( + status_code=409, + detail=( + "Force-push rejected: another instance has " + "written to this branch since the last fetch. " + "A collision alert was recorded in the operator " + "queue. Rebind one of the agents to a fresh " + "working branch before retrying." + ), + headers={"X-Conflict-Type": "branch_ownership_collision"}, + ) + raise HTTPException( + status_code=500, + detail=f"Force push failed: {stderr}", + ) else: # Normal push or pull_first (after pull, should be safe to push) push_result = subprocess.run( diff --git a/docs/memory/feature-flows/github-repo-initialization.md b/docs/memory/feature-flows/github-repo-initialization.md index 25bed60a1..49753c7a4 100644 --- a/docs/memory/feature-flows/github-repo-initialization.md +++ b/docs/memory/feature-flows/github-repo-initialization.md @@ -12,6 +12,7 @@ As a **Trinity platform user**, I want to **enable GitHub synchronization for an | Date | Changes | |------|---------| +| 2026-04-19 | S7 Layers 0/2 (#382): consolidated three independent `generate_instance_id()` call sites behind new `reserve_and_generate_instance_id()` helper in `services/git_service.py:115-206`. Reservation now atomically (a) generates a UUID, (b) probes the remote with `git ls-remote --heads --exit-code` (Layer 1), and (c) inserts the `agent_git_config` row under a new partial UNIQUE index `UNIQUE(github_repo, working_branch) WHERE source_mode = 0` (Layer 2). Retries up to `MAX_INSTANCE_ID_RETRIES` (5) on either remote or DB collision before raising `RuntimeError`. The reservation is performed BEFORE container creation in `agent_service/crud.py` and BEFORE `initialize_git_in_container` in `routers/git.py`, and is rolled back via `db.delete_git_config(...)` if any later step fails so a retry can claim a fresh branch. The duplicate post-container `db.create_git_config` call previously made by `crud.py` was removed. Schema added in `db/schema.py:757-758`; existing databases get the index via migration `agent_git_config_branch_ownership` (`db/migrations.py:1454-1511`), which refuses to install when `_find_duplicate_working_branches` finds existing collisions and prints every offending row so the operator can rebind one of the duplicates first. Failure mode: agent creation that previously could silently bind to an in-use working branch now fails fast with `RuntimeError` after 5 reservation attempts. Regression coverage: `tests/git-sync/test_s7_reserve_instance_id.py`. (Layer 3 push-time `--force-with-lease` guard documented in `github-sync.md`.) | | 2026-04-10 | Fix (#256): `initialize_git_in_container` now pushes after commit in the `remote_has_main` code path. Previously the workspace was committed locally inside the container but never reached GitHub when the target repo already had a `main` branch, making the UI report success while no files synced. | | 2026-02-11 | Added LEGACY notes for workspace path checks - new agents (2026-02+) use `/home/developer` directly, workspace checks exist for backward compatibility with pre-2026-02 agents | | 2026-01-23 | Verified line numbers against current implementation, updated MCP tool lines (530-604), verified git_service.py structure | @@ -109,8 +110,27 @@ sequenceDiagram GitHubService->>Backend: GitHubRepoInfo(exists=True) end - Note over Backend,Container: Phase 4: Git Initialization (via git_service) - Backend->>GitService: initialize_git_in_container() + Note over Backend,Container: Phase 4a: Reserve Working Branch (S7 Layer 0) + Backend->>GitService: reserve_and_generate_instance_id(agent, repo) + loop up to MAX_INSTANCE_ID_RETRIES (5) + GitService->>GitService: generate_instance_id() (UUID prefix) + GitService->>GitService: working_branch = trinity/{agent}/{id} + GitService->>GitHub: git ls-remote --heads --exit-code (Layer 1) + alt Remote already has branch + Note over GitService: Retry with fresh UUID + else Remote clear + GitService->>GitService: db.create_git_config() (Layer 2 partial UNIQUE) + alt sqlite3.IntegrityError on (repo, branch) + Note over GitService: Retry with fresh UUID + else Insert succeeds + GitService->>Backend: (instance_id, working_branch) + end + end + end + Note over GitService: After 5 collisions: raise RuntimeError + + Note over Backend,Container: Phase 4b: Git Initialization (via git_service) + Backend->>GitService: initialize_git_in_container(working_branch=reserved, create_working_branch=False) GitService->>Container: Check for existing git directory alt Has existing /home/developer/workspace with content (LEGACY) Note over GitService: LEGACY: Agents created before 2026-02 @@ -120,14 +140,13 @@ sequenceDiagram GitService->>Container: Create .gitignore end GitService->>Container: Execute git init, add, commit, push - GitService->>Container: Create working branch + GitService->>Container: Check out + push pre-reserved working branch GitService->>Container: Verify git directory GitService->>Backend: GitInitResult(success=True, git_dir, working_branch) + Note over Backend: On any failure: db.delete_git_config(agent) to release reservation - Note over Backend,Container: Phase 5: Persistence - Backend->>GitService: create_git_config_for_agent() - GitService->>GitService: Store in agent_git_config table - Backend->>Store: Return success + repo details + Note over Backend,Container: Phase 5: Response + Backend->>Store: Return success + repo details (config row already persisted in Phase 4a) Store->>GitPanel: Close modal GitPanel->>GitPanel: Reload git status GitPanel->>User: Show git sync UI with remote URL @@ -351,17 +370,44 @@ class GitHubService: | Lines | Component | Description | |-------|-----------|-------------| -| 22-24 | `generate_instance_id()` | Generate 8-char UUID for instance | -| 27-29 | `generate_working_branch()` | Generate branch name `trinity/{name}/{id}` | -| 32-61 | `create_git_config_for_agent()` | Create database git config record | -| 253-259 | `GitInitResult` dataclass | Result of git init in container | -| 262-399 | `initialize_git_in_container()` | Full git init workflow | -| 402-427 | `check_git_initialized()` | Check if git exists in container | - -**Key Function**: +| 29 | `MAX_INSTANCE_ID_RETRIES` | Max reservation retries before `RuntimeError` (5) — S7 Layer 0 | +| 32-40 | `generate_instance_id()` | Raw 8-char UUID prefix. Kept for the reserve helper; new callers MUST use `reserve_and_generate_instance_id` | +| 43-45 | `generate_working_branch()` | Generate branch name `trinity/{name}/{id}` | +| 48-112 | `check_remote_branch_exists()` | `git ls-remote --heads --exit-code` probe (S7 Layer 1) — fails-open on network/timeout because Layer 2 is the real guarantee | +| 115-206 | `reserve_and_generate_instance_id()` | **S7 Layer 0 single entry point**: UUID + remote probe + DB insert under partial UNIQUE index. Retries `MAX_INSTANCE_ID_RETRIES` times then raises `RuntimeError` | +| 209-238 | `create_git_config_for_agent()` | Legacy create-without-reserve helper (still used in tests/non-S7 paths) | +| 430-436 | `GitInitResult` dataclass | Result of git init in container | +| 439-658 | `initialize_git_in_container()` | Full git init workflow. New `working_branch` kwarg accepts pre-reserved branch; `create_working_branch=True` is deprecated and logs a warning | +| 661-690 | `check_git_initialized()` | Check if git exists in container | + +**Key Functions**: ```python -# Line 253-259: GitInitResult dataclass +# Line 115-206: S7 Layer 0 single entry point — atomic reserve +async def reserve_and_generate_instance_id( + agent_name: str, + github_repo: str, + source_branch: str = "main", + source_mode: bool = False, + sync_paths: Optional[List[str]] = None, +) -> Tuple[str, str]: + """Atomically reserve a fresh working branch. + + Combines: + 1. UUID generation + 2. ``git ls-remote`` probe (Layer 1) + 3. DB insert into ``agent_git_config`` under the partial UNIQUE + index UNIQUE(github_repo, working_branch) WHERE source_mode = 0 + (Layer 2) + + Retries on either remote or DB collision up to + MAX_INSTANCE_ID_RETRIES (5) before raising RuntimeError. + For source_mode=True the remote probe is skipped (intentional shared + branch) and the DB insert bypasses the partial UNIQUE index by design. + """ + + +# Line 430-436: GitInitResult dataclass @dataclass class GitInitResult: """Result of git initialization in container.""" @@ -371,12 +417,13 @@ class GitInitResult: error: Optional[str] = None -# Line 262-399: Initialize git in container +# Line 439-658: Initialize git in container (S7 Layer 0 signature) async def initialize_git_in_container( agent_name: str, github_repo: str, github_pat: str, - create_working_branch: bool = True + create_working_branch: bool = True, # DEPRECATED — emits warning + working_branch: Optional[str] = None, # NEW: pre-reserved branch ) -> GitInitResult: """ Initialize git in an agent container. @@ -388,7 +435,10 @@ async def initialize_git_in_container( 4. Configure remote 5. Create initial commit 6. Push to GitHub - 7. Create working branch (optional) + 7. Create working branch — prefers the pre-reserved path: + - working_branch= (preferred) → just check out + push + - create_working_branch=True (legacy) → generates an instance ID + internally, bypassing the Layer 0/1/2 guarantees. Warning logged. """ container_name = f"agent-{agent_name}" @@ -500,7 +550,9 @@ OwnedAgentByName = Annotated[str, Depends(get_owned_agent_by_name)] | Lines | Component | Description | |-------|-----------|-------------| | 34-41 | `GitInitializeRequest` model | Request body schema | -| 251-389 | `POST /{agent_name}/git/initialize` | Main initialization endpoint | +| 251-410 | `POST /{agent_name}/git/initialize` | Main initialization endpoint (S7 Layer 0 reserve-then-init) | +| 346-349 | `reserve_and_generate_instance_id()` call | Atomic reservation BEFORE container init | +| 351-394 | `try`/`except` around `initialize_git_in_container` | Rolls back via `db.delete_git_config` if init fails so retries can claim a fresh branch | **Endpoint Implementation**: @@ -562,30 +614,42 @@ async def initialize_github_sync( if not repo_info.exists: raise HTTPException(status_code=400, detail=f"Repository '{repo_full_name}' does not exist...") - # Step 2: Initialize git in container (lines 338-364) - init_result = await git_service.initialize_git_in_container( + # Step 2: Reserve the working branch BEFORE touching the container. + # S7 Layer 0 (#382): single-entry helper — remote probe + DB insert + # under the partial UNIQUE index in one shot. Raises RuntimeError + # after MAX_INSTANCE_ID_RETRIES collisions. + instance_id, reserved_branch = await git_service.reserve_and_generate_instance_id( agent_name=agent_name, github_repo=repo_full_name, - github_pat=github_pat ) - if not init_result.success: - # Handle various error types with appropriate status codes - raise HTTPException(status_code=400, detail=f"Git initialization failed: {init_result.error}") - - # Step 3: Store configuration (lines 366-372) - instance_id = git_service.generate_instance_id() - config = await git_service.create_git_config_for_agent( - agent_name=agent_name, - github_repo=repo_full_name, - instance_id=instance_id - ) + try: + # Step 3: Initialize git in container using the reserved branch. + # create_working_branch=False tells the helper not to generate + # its own ID — the caller owns the reservation now. + init_result = await git_service.initialize_git_in_container( + agent_name=agent_name, + github_repo=repo_full_name, + github_pat=github_pat, + create_working_branch=False, + working_branch=reserved_branch, + ) + + if not init_result.success: + raise HTTPException(status_code=400, detail=f"Git initialization failed: {init_result.error}") + except Exception: + # Release the reservation so a retry can grab a fresh branch. + try: + db.delete_git_config(agent_name) + except Exception as cleanup_exc: + logger.warning("Failed to roll back agent_git_config for %s after init failure: %s", agent_name, cleanup_exc) + raise return { "success": True, "message": "GitHub sync initialized successfully", "github_repo": repo_full_name, - "working_branch": init_result.working_branch, + "working_branch": reserved_branch, "instance_id": instance_id, "repo_url": f"https://github.com/{repo_full_name}" } @@ -729,14 +793,28 @@ initializeGithubSync: { | Column | Type | Description | |--------|------|-------------| -| `agent_name` | TEXT PRIMARY KEY | Agent identifier | +| `id` | TEXT PRIMARY KEY | Row id | +| `agent_name` | TEXT UNIQUE NOT NULL | Agent identifier | | `github_repo` | TEXT | Full repo name (owner/name) | | `working_branch` | TEXT | Branch name (trinity/{name}/{id}) | | `instance_id` | TEXT | Unique instance identifier (8 chars) | +| `source_branch` | TEXT | Source branch (default `'main'`) | +| `source_mode` | INTEGER | 0 = working-branch agent, 1 = read-only source-mode | | `created_at` | TIMESTAMP | When config was created | | `last_sync_at` | TIMESTAMP | Last successful sync | | `last_commit_sha` | TEXT | Last synced commit SHA | | `sync_enabled` | BOOLEAN | Whether sync is active | +| `sync_paths` | TEXT | Optional JSON list of allowed paths | +| `github_pat_encrypted` | TEXT | Per-agent PAT (#347) | + +**Indexes**: +- `idx_git_config_agent` on `agent_name` +- `idx_git_config_repo` on `github_repo` +- `idx_git_config_repo_branch_unique` — **S7 Layer 2 partial UNIQUE**: + `UNIQUE(github_repo, working_branch) WHERE source_mode = 0`. Defined in + `src/backend/db/schema.py:757-758` for fresh installs and added to existing + databases by migration `agent_git_config_branch_ownership` + (`src/backend/db/migrations.py:1454-1511`). **Table**: `system_settings` @@ -1307,6 +1385,66 @@ trinity/{agent-name}/{instance-id} - Easy to create PRs from working branch to main - Instance ID ensures uniqueness across restarts +### Branch Ownership (S7 Layers 0 & 2 — #382, 2026-04-19) + +Working-branch agents must be exclusively bound to their `(github_repo, +working_branch)` pair. This is enforced in three layers; Layers 0 and 2 +live in this flow (Layer 3, the push-time `--force-with-lease` guard, +lives in [github-sync.md](github-sync.md)). + +**Layer 0 — single-entry reservation helper** +- `services/git_service.py:reserve_and_generate_instance_id()` (lines 115-206) + is the only sanctioned way to allocate a working branch. +- Atomically: generates a UUID prefix → probes the remote with + `git ls-remote --heads --exit-code` → inserts into `agent_git_config`. +- Retries up to `MAX_INSTANCE_ID_RETRIES` (5, defined at line 29) on any + collision before raising `RuntimeError`. +- Source-mode agents (`source_mode=True`) intentionally share branches + (e.g. every reader pointing at `main`); the remote probe is skipped and + the DB insert bypasses the partial UNIQUE index by design. + +**Layer 1 — remote probe** +- `services/git_service.py:check_remote_branch_exists()` (lines 48-112). +- Uses `https://github.com/.git` so the check works without a PAT + for public repos. Fails open (returns `False`) on network errors, + timeout, or `git` missing — Layer 2 is the real guarantee. + +**Layer 2 — partial UNIQUE index** +- `db/schema.py:757-758` for fresh installs. +- Migration `agent_git_config_branch_ownership` in + `db/migrations.py:1454-1511` for existing databases. +- Pre-flight `_find_duplicate_working_branches()` + (`db/migrations.py:1411-1451`) scans for pre-existing duplicates. + If any are found the migration **refuses to install the index** and + prints every offending `(repo, branch, agents)` group plus each + individual agent name so the operator can rebind one of the colliding + agents (via the UI / MCP `initialize_github_sync` flow) before + re-running. The migration **never auto-deletes rows** — that would + mask the bug. + +**Call-site consolidation** +| Caller | Location | Behavior change | +|--------|----------|-----------------| +| Agent creation | `services/agent_service/crud.py:232-239` | Reserves BEFORE container creation. The duplicate post-container `db.create_git_config` (previously around line 619) was removed. On any failure later in the function the `except` block at lines 626-639 calls `db.delete_git_config(config.name)` to release the reservation. | +| `POST /git/initialize` | `routers/git.py:346-394` | Reserves BEFORE `initialize_git_in_container`. Passes `working_branch=, create_working_branch=False`. Wraps the container init in a `try`/`except` that rolls back the reservation on failure. | +| `initialize_git_in_container` | `services/git_service.py:439-658` | Adds `working_branch: Optional[str] = None` kwarg for the pre-reserved path (lines 595-611). The legacy `create_working_branch=True` path (lines 612-636) emits a deprecation warning on every use but is kept so older callers don't break. | + +**Failure mode** +Agent creation that previously could silently bind to an in-use working +branch (the 2026-04-17 alpaca incident) now fails fast with a +`RuntimeError` if all 5 reservation attempts collide. The partial +transaction is rolled back; the caller is expected to retry agent +creation, which will pick a fresh UUID. + +**Regression coverage** +`tests/git-sync/test_s7_reserve_instance_id.py` covers: +- (i) `reserve_and_generate_instance_id` retries on `ls-remote` hits +- (ii) raises after `MAX_INSTANCE_ID_RETRIES` +- (iii) the partial UNIQUE index rejects duplicate working-branch + bindings and accepts duplicates for source-mode agents +- (iv) the migration pre-flight surfaces existing duplicates and + refuses to install the index until they're resolved + ### Repository Structure After initialization: diff --git a/docs/memory/feature-flows/github-sync.md b/docs/memory/feature-flows/github-sync.md index 65cc156b4..c3998b6ec 100644 --- a/docs/memory/feature-flows/github-sync.md +++ b/docs/memory/feature-flows/github-sync.md @@ -238,9 +238,9 @@ sequenceDiagram Backend->>Container: POST /api/git/sync Container->>Container: git add -A Container->>Container: git commit -m "Trinity sync: {timestamp}" - Container->>Container: git push --force-with-lease - Container->>Backend: Return commit SHA - Backend->>UI: Show notification + Container->>Container: git push --force-with-lease={branch}:{expected-sha} + Container->>Backend: Return commit SHA (or 409 on stale lease) + Backend->>UI: Show notification (or branch_ownership_collision modal) ``` --- @@ -760,7 +760,7 @@ Operators staring at `merge_conflict` and a wall of raw git stderr could not tel |-----------|------|------------|-------------| | Modal | `src/frontend/src/components/GitConflictModal.vue` | - | Conflict resolution UI | | Composable | `src/frontend/src/composables/useGitSync.js` | 1-202 | State management with conflict handling | -| Agent-Server | `docker/base-image/agent_server/routers/git.py` | 1-644 | Git operations with strategies | +| Agent-Server | `docker/base-image/agent_server/routers/git.py` | 1-854 | Git operations with strategies + S7 Layer 3 lease/collision surfacing | | Backend Router | `src/backend/routers/git.py` | 1-389 | API endpoints with strategy params | | Git Service | `src/backend/services/git_service.py` | 1-427 | Proxy to agent with conflict detection | | Settings Service | `src/backend/services/settings_service.py` | 71-76, 113-115 | GitHub PAT retrieval | @@ -790,10 +790,91 @@ def _get_pull_branch(current_branch: str, home_dir: Path) -> str: | Endpoint | Line Range | Description | |----------|------------|-------------| -| `GET /api/git/status` | 86-251 | Get repository status, branch, changes, ahead/behind (uses `_get_pull_branch` for behind count); also surfaces `pull_branch`, `common_ancestor_sha`, `common_ancestor_age_days` for parallel-history detection (S2, #385) | -| `POST /api/git/sync` | 254-407 | Stage, commit, push with strategy support (`pull_first` pulls from `pull_branch`) | -| `GET /api/git/log` | 410-457 | Get recent commit history | -| `POST /api/git/pull` | 460-659 | Pull from remote with conflict strategies (targets `pull_branch`) | +| `GET /api/git/status` | — | Get repository status, branch, changes, ahead/behind (uses `_get_pull_branch` for behind count); surfaces `pull_branch`, `common_ancestor_sha`, `common_ancestor_age_days` for parallel-history detection (S2, #385); persists last-remote-sha after fetch (S7 Layer 3, #382) | +| `POST /api/git/sync` | — | Stage, commit, push with strategy support (`pull_first` pulls from `pull_branch`); `force_push` uses `--force-with-lease=:` (S7 Layer 3, #382) | +| `GET /api/git/log` | — | Get recent commit history | +| `POST /api/git/pull` | — | Pull from remote with conflict strategies (targets `pull_branch`) | + +--- + +## Branch-Ownership Collision Handling (S7 Layer 3, #382) + +When two Trinity instances bind to the same working branch, only one can push without clobbering the other. Layers 0 and 2 (reservation helper + partial UNIQUE index — see [github-repo-initialization.md](github-repo-initialization.md)) prevent the binding from being created in the first place. Layer 3 — the push-time guard documented here — is the last line of defense for instances that already have a stale binding from before those constraints existed. + +**2026-04-17 incident reference**: This Layer 3 change is the response to the alpaca-vybe-live silent clobber, where a peer instance's `git push --force` overwrote in-flight work without either side noticing. Tier-1 regression coverage lives at `tests/git-sync/test_p5_branch_ownership.sh`. + +### Persisted last-remote-sha (the lease) + +After every successful `git fetch`, the agent server records the observed remote SHA to `~/.trinity/last-remote-sha/` (file path mirrors the branch layout, so `trinity//` becomes nested directories). The next push reads that value and passes it as the `expected-sha` argument to `--force-with-lease`. + +| Helper | File | Lines | Purpose | +|--------|------|-------|---------| +| `LAST_REMOTE_SHA_DIR` | `docker/base-image/agent_server/routers/git.py` | 18-23 | `~/.trinity/last-remote-sha` (under bind-mounted home, survives restarts) | +| `_sha_file_for_branch()` | `docker/base-image/agent_server/routers/git.py` | 46-53 | Mirrors branch layout as nested dirs | +| `_persist_last_remote_sha()` | `docker/base-image/agent_server/routers/git.py` | 56-94 | Called after fetch; failure is logged, never raised | +| `_read_last_remote_sha()` | `docker/base-image/agent_server/routers/git.py` | 97-106 | Read on push path; returns `None` if missing | + +**Persist call sites** (after every successful fetch): +- `get_git_status()` — line 249-250 (after the status-path fetch) +- `sync_to_github()` `pull_first` strategy — line 353-354 (after the pre-sync fetch) + +**Failure mode**: if persistence fails (disk full, permission error), the next push has no lease on file and falls back to the unparameterized `git push --force-with-lease`, which uses the remote-tracking ref as the expected-sha. Still safer than plain `--force`; degraded for one push then self-heals on the next fetch. + +### Force-push with lease + +`sync_to_github()` `force_push` strategy (lines 489-536): + +```python +lease_sha = _read_last_remote_sha(current_branch) +push_cmd: list[str] = ["git", "push"] +if lease_sha: + push_cmd.append(f"--force-with-lease={current_branch}:{lease_sha}") +else: + push_cmd.append("--force-with-lease") # remote-tracking-ref fallback +push_cmd += ["origin", current_branch] +``` + +If a peer instance pushed to the branch since this instance last fetched, `origin/` is now ahead of `lease_sha`, the lease is stale, and `git push` exits non-zero with `stderr` containing `stale info`. `_is_stale_lease_rejection()` (lines 168-171) detects that signature. + +### HTTP 409 + collision header + +On lease rejection, the endpoint returns: + +``` +HTTP/1.1 409 Conflict +X-Conflict-Type: branch_ownership_collision + +{"detail": "Force-push rejected: another instance has written to this branch since the last fetch. ..."} +``` + +(`agent_server/routers/git.py:522-532`.) The new `branch_ownership_collision` value is distinct from the existing `push_rejected` (non-fast-forward without `force_push`) and `merge_conflict` cases, so the frontend can show a dedicated remediation modal instead of offering a "force push again" button (which would fail the same way). + +### Operator-queue collision alert + +`_record_push_collision()` (lines 109-165) appends a structured `alert` entry to `~/.trinity/operator-queue.json` when the lease is rejected: + +```json +{ + "id": "git-collision-", + "type": "alert", + "status": "pending", + "priority": "high", + "title": "Git push rejected on — branch binding collision", + "question": "Another Trinity instance wrote to this working branch since this agent last fetched. ...", + "options": null, + "context": { + "branch": "", + "expected_sha": "", + "git_stderr": "", + "remediation": "Assign a fresh working branch to one of the colliding agents (Fleet → Branch Bindings → Assign fresh branch)." + }, + "created_at": "" +} +``` + +The backend's `OperatorQueueSyncService` (5s polling, see `services/operator_queue_service.py`) picks this up on its next sweep and surfaces it in the Operating Room. The losing instance now learns it lost the race, instead of believing its push succeeded — that's the substantive behavior change versus the pre-#382 silent-clobber regime. + +Append failures are caught and logged but never raised — surfacing the alert is best-effort; the 409 to the caller is the authoritative signal. #### `GET /api/git/status` — parallel-history fields (S2, #385) @@ -817,7 +898,7 @@ Push is the right answer: 1. **GitHub PAT**: Passed as environment variable, never exposed in logs or API responses 2. **Remote URL Sanitization**: Credentials stripped before display -3. **Force Push Protection**: Uses `--force-with-lease` for normal pushes +3. **Force Push Protection**: All pushes (normal AND `force_push` strategy) use `--force-with-lease`. `force_push` parameterizes the lease with the persisted last-observed remote SHA so a stale binding cannot clobber a peer (S7 Layer 3, #382). See "Branch-Ownership Collision Handling" above. 4. **Force Operations Warning**: UI shows red destructive warnings for force operations 5. **Infrastructure Files**: `content/`, `.local/` auto-added to `.gitignore` 6. **Access Control**: Read endpoints and pull use `AuthorizedAgentByName` (owner/shared/admin), write endpoints (sync/initialize) use `OwnedAgentByName` (owner/admin only) @@ -845,8 +926,9 @@ Working - Pull fix for working branches (2026-03-26) | Date | Changes | |------|---------| -| 2026-04-19 | **S5 — operator-readable conflict diagnosis** (#386, PR #397): Added `ConflictClass` enum + pure `classify_conflict()` in `src/backend/services/git_service.py:28-130`, mirrored in new `docker/base-image/agent_server/utils/git_conflict.py`. New `conflict_class` field on `GitSyncResult` (`db_models.py:231`) and `X-Conflict-Class` response header on 409s (`routers/git.py:134-140, 212-218`). Agent-server collapsed 5 `HTTPException` sites behind a shared `_conflict_response()` helper (`docker/base-image/agent_server/routers/git.py:41-68`). `GitConflictModal.vue` picks per-class title/body/recommendation from a `COPY` lookup; raw stderr in an expandable `
`; pre-S5 fallback preserves old strings. Composable `useGitSync.js:87-93, 131-138` reads the header into the existing `gitConflict` ref. Regression coverage in `tests/git-sync/test_s5_conflict_classifier.py` (11 pytest cases) and `tests/git-sync/test_s5_modal.spec.js` (5 Vitest snapshots). | -| 2026-04-19 | **S2 — Parallel-history detection at modal open** (#385, PR #395): `get_git_status()` in `docker/base-image/agent_server/routers/git.py` now also returns `pull_branch` (line 235), `common_ancestor_sha` (line 242), and `common_ancestor_age_days` (line 243), computed via `git merge-base HEAD origin/` (line 184-192) and `git log -1 --format=%cI` (line 194-213). Existing `ahead`/`behind` are unchanged. Frontend `useGitSync.js` adds `pullBranch` (lines 36-38), `isParallelHistory` predicate (lines 44-55, threshold 30 days), and an `adoptUpstreamPreserveState()` resolver (lines 189-220) that POSTs to `/api/agents/{name}/git/reset-to-main-preserve-state` (S3 / #384, not yet live — surfaces a clear "blocked on #384" notification on 404 / missing store method). `GitConflictModal.vue` adds a sibling `
` variant (lines 106-184) titled "Your agent cannot sync" with primary "Adopt latest upstream (preserve my state)" and secondary "Force push anyway"; the existing variant's outer `v-if` is narrowed to `show && !isParallelHistory` so the new branch takes priority. All copy uses `{{ pullBranchLabel }}` — no hardcoded "main". `AgentDetail.vue` forwards the two new reactives into the modal mount. Regression tests: `tests/git-sync/test_p2_parallel_history_detection.sh` (in-process FastAPI) and `tests/git-sync/test_s2_usegitsync.test.js` (Vitest predicate scenarios). | +| 2026-04-19 | **S7 Layer 3 push-time guard** (#382, PR #396): `sync_to_github()` `force_push` strategy now uses `git push --force-with-lease=:` instead of plain `--force` (`docker/base-image/agent_server/routers/git.py`). The expected-sha is the remote value last observed at fetch, persisted to `~/.trinity/last-remote-sha/` via `_persist_last_remote_sha()` after every successful fetch. Stale-lease rejection returns HTTP 409 with header `X-Conflict-Type: branch_ownership_collision` and appends a structured alert to `~/.trinity/operator-queue.json` (`_record_push_collision()`), which the backend's `OperatorQueueSyncService` surfaces in the Operating Room on its next 5s poll. Persistence/append failures are logged, never raised. Response to the 2026-04-17 alpaca-vybe-live silent clobber; Tier-1 regression at `tests/git-sync/test_p5_branch_ownership.sh`. Layers 0 and 2 (reservation helper + partial UNIQUE index) are documented in `github-repo-initialization.md`. | +| 2026-04-19 | **S5 — operator-readable conflict diagnosis** (#386, PR #397): Added `ConflictClass` enum + pure `classify_conflict()` in `src/backend/services/git_service.py`, mirrored in new `docker/base-image/agent_server/utils/git_conflict.py`. New `conflict_class` field on `GitSyncResult` and `X-Conflict-Class` response header on 409s. Agent-server collapsed 5 `HTTPException` sites behind a shared `_conflict_response()` helper. `GitConflictModal.vue` picks per-class title/body/recommendation from a `COPY` lookup; raw stderr in an expandable `
`; pre-S5 fallback preserves old strings. Composable `useGitSync.js` reads the header into the existing `gitConflict` ref. Regression coverage in `tests/git-sync/test_s5_conflict_classifier.py` (11 pytest cases) and `tests/git-sync/test_s5_modal.spec.js` (5 Vitest snapshots). | +| 2026-04-19 | **S2 — Parallel-history detection at modal open** (#385, PR #395): `get_git_status()` in `docker/base-image/agent_server/routers/git.py` now also returns `pull_branch`, `common_ancestor_sha`, and `common_ancestor_age_days`, computed via `git merge-base HEAD origin/` and `git log -1 --format=%cI`. Existing `ahead`/`behind` are unchanged. Frontend `useGitSync.js` adds `pullBranch`, `isParallelHistory` predicate (threshold 30 days), and an `adoptUpstreamPreserveState()` resolver that POSTs to `/api/agents/{name}/git/reset-to-main-preserve-state` (S3 / #384, not yet live — surfaces a clear "blocked on #384" notification on 404 / missing store method). `GitConflictModal.vue` adds a sibling `
` variant titled "Your agent cannot sync" with primary "Adopt latest upstream (preserve my state)" and secondary "Force push anyway"; the existing variant's outer `v-if` is narrowed to `show && !isParallelHistory` so the new branch takes priority. All copy uses `{{ pullBranchLabel }}` — no hardcoded "main". `AgentDetail.vue` forwards the two new reactives into the modal mount. Regression tests: `tests/git-sync/test_p2_parallel_history_detection.sh` and `tests/git-sync/test_s2_usegitsync.test.js`. | | 2026-04-16 | **Per-Agent GitHub PAT** (#347): Added per-agent PAT configuration. Agents can now use their own GitHub PAT instead of the global one. DB column `github_pat_encrypted` in `agent_git_config`, encrypted with AES-256-GCM. 3 new API endpoints (GET/PUT/DELETE), 2 MCP tools, GitPanel.vue settings UI. Helper `get_github_pat_for_agent()` provides fallback logic. | | 2026-03-26 | **Fix git pull for working branches** (#195): Added `_get_pull_branch()` helper that detects `trinity/*` branches and redirects pull/status to `origin/main`. Fixed `git fetch --dry-run` → `git fetch origin` in status endpoint. Fixed `initialize_git_in_container()` to preserve remote history via `git fetch + reset` instead of `git init + force push`. Tests in `tests/unit/test_git_pull_branch.py`. | | 2026-02-28 | **Git Branch Support** (GIT-002): Added complete data flow documentation with line numbers. URL syntax (`github:owner/repo@branch`) parses branch in crud.py:102-113. MCP types.ts:29 and agents.ts:201-207 expose `source_branch` parameter. template_service.py:22-41 passes branch to git clone. startup.sh:38-45 uses `-b` flag. Added testing checklist from requirements spec. | diff --git a/docs/memory/requirements.md b/docs/memory/requirements.md index 16b9f195a..3b3ce8085 100644 --- a/docs/memory/requirements.md +++ b/docs/memory/requirements.md @@ -446,6 +446,16 @@ Trinity is autonomous agent orchestration and infrastructure — sovereign infra - **GitHub Issue**: #385 (Epic #381) - **Flow**: `docs/memory/feature-flows/github-sync.md` +### 11.5 Branch Ownership Enforcement (S7) +- **Status**: ✅ Implemented (2026-04-19) +- **Description**: Prevent silent data loss from two agents binding to the same `(github_repo, working_branch)` pair (2026-04-17 alpaca incident). +- **Key Features**: + - Layer 0/1: single `reserve_and_generate_instance_id` helper atomically generates a UUID, probes `git ls-remote`, and inserts under the partial UNIQUE; retries 5x on collision. + - Layer 2: partial UNIQUE `(github_repo, working_branch) WHERE source_mode = 0` on `agent_git_config`; migration refuses to install on existing duplicates so operators rebind first. + - Layer 3: agent-server pushes with `git push --force-with-lease=:`; lease rejection returns 409 + `X-Conflict-Type: branch_ownership_collision` and emits a structured alert into `~/.trinity/operator-queue.json` so the Operating Room surfaces the collision. +- **GitHub Issue**: #382 (Epic #381) +- **Flows**: `docs/memory/feature-flows/github-repo-initialization.md`, `docs/memory/feature-flows/github-sync.md` + --- ## 12. Platform Operations diff --git a/src/backend/db/migrations.py b/src/backend/db/migrations.py index 37b83d323..a0f1c43fe 100644 --- a/src/backend/db/migrations.py +++ b/src/backend/db/migrations.py @@ -1408,6 +1408,109 @@ def _migrate_proactive_messaging(cursor, conn): conn.commit() +def _find_duplicate_working_branches(cursor): + """Return groups of (github_repo, working_branch) pairs bound to >1 agent + with source_mode=0. + + S7 Layer 2 pre-flight. Duplicates here are the exact bug the 2026-04-17 + alpaca incident was caused by, and they block the UNIQUE index from + being created on an existing database. The migration surfaces them so + an operator can rebind the losing agent to a fresh working branch + before re-running. + + Returns: + List of dicts: ``{github_repo, working_branch, agent_names}``. + Empty list if no duplicates. + """ + cursor.execute( + """ + SELECT github_repo, working_branch, GROUP_CONCAT(agent_name, ',') AS agents, + COUNT(*) AS n + FROM agent_git_config + WHERE source_mode = 0 + GROUP BY github_repo, working_branch + HAVING n > 1 + ORDER BY github_repo, working_branch + """ + ) + rows = cursor.fetchall() + + duplicates = [] + for row in rows: + github_repo = row[0] if not hasattr(row, "keys") else row["github_repo"] + working_branch = row[1] if not hasattr(row, "keys") else row["working_branch"] + agents_csv = row[2] if not hasattr(row, "keys") else row["agents"] + agent_names = sorted(agents_csv.split(",")) if agents_csv else [] + duplicates.append( + { + "github_repo": github_repo, + "working_branch": working_branch, + "agent_names": agent_names, + } + ) + return duplicates + + +def _migrate_agent_git_config_branch_ownership(cursor, conn): + """Add partial UNIQUE index to agent_git_config enforcing branch ownership (S7 Layer 2 / #382). + + Enforces ``UNIQUE(github_repo, working_branch) WHERE source_mode = 0``, + which makes it impossible for two working-branch agents to be bound to + the same (repo, branch) pair. Source-mode agents intentionally share + branches (e.g. every reader pointing at ``main``) and are excluded from + the constraint. + + Pre-flight: if the database already contains duplicate bindings (e.g. + from the 2026-04-17 alpaca incident), the migration refuses to create + the index and raises with an actionable error that names every + offending row. The operator must rebind duplicates to fresh working + branches (via the UI / MCP ``initialize_github_sync`` flow) before + re-running. We never auto-delete rows — that would mask the bug. + """ + # Idempotency: skip if the index is already present. + cursor.execute( + "SELECT name FROM sqlite_master " + "WHERE type='index' AND name='idx_git_config_repo_branch_unique'" + ) + if cursor.fetchone(): + return + + duplicates = _find_duplicate_working_branches(cursor) + if duplicates: + report_lines = [ + "Cannot enforce S7 branch-ownership UNIQUE index — " + "found existing duplicate bindings:", + ] + for group in duplicates: + report_lines.append( + f" - repo={group['github_repo']!r} " + f"branch={group['working_branch']!r} " + f"agents={group['agent_names']}" + ) + # Also log each individual agent name so tests / log search + # can find them easily. + for agent in group["agent_names"]: + report_lines.append(f" offending agent: {agent}") + report_lines.append( + "Resolve by assigning a fresh working branch to all but one " + "agent in each group (see docs/memory/feature-flows/ " + "github-sync.md), then re-run the migration." + ) + message = "\n".join(report_lines) + # Print so the operator sees it even if logging is suppressed. + print(message) + logger.error(message) + raise RuntimeError(message) + + cursor.execute( + "CREATE UNIQUE INDEX IF NOT EXISTS idx_git_config_repo_branch_unique " + "ON agent_git_config(github_repo, working_branch) WHERE source_mode = 0" + ) + conn.commit() + print("Created S7 partial UNIQUE index idx_git_config_repo_branch_unique " + "on agent_git_config(github_repo, working_branch) WHERE source_mode = 0") + + MIGRATIONS = [ ("agent_sharing", _migrate_agent_sharing_table), ("schedule_executions_observability", _migrate_schedule_executions_observability), @@ -1456,4 +1559,5 @@ def _migrate_proactive_messaging(cursor, conn): ("agent_ownership_guardrails", _migrate_agent_ownership_guardrails), ("agent_git_config_pat", _migrate_agent_git_config_pat), ("proactive_messaging", _migrate_proactive_messaging), + ("agent_git_config_branch_ownership", _migrate_agent_git_config_branch_ownership), ] diff --git a/src/backend/db/schema.py b/src/backend/db/schema.py index 5d67135d9..10ca15ee3 100644 --- a/src/backend/db/schema.py +++ b/src/backend/db/schema.py @@ -749,6 +749,13 @@ # Git config indexes "CREATE INDEX IF NOT EXISTS idx_git_config_agent ON agent_git_config(agent_name)", "CREATE INDEX IF NOT EXISTS idx_git_config_repo ON agent_git_config(github_repo)", + # S7 Layer 2: a working branch may only be bound to one agent within a + # given repo. Source-mode agents intentionally share a branch (e.g. every + # reader points at `main`) so the index is partial and excludes them. + # See src/backend/db/migrations.py::_migrate_agent_git_config_branch_ownership + # for the operator-assisted migration path on existing databases. + "CREATE UNIQUE INDEX IF NOT EXISTS idx_git_config_repo_branch_unique " + "ON agent_git_config(github_repo, working_branch) WHERE source_mode = 0", # Chat session indexes "CREATE INDEX IF NOT EXISTS idx_chat_sessions_agent ON chat_sessions(agent_name)", diff --git a/src/backend/routers/git.py b/src/backend/routers/git.py index b0e974f9a..ea2fc9947 100644 --- a/src/backend/routers/git.py +++ b/src/backend/routers/git.py @@ -7,6 +7,7 @@ - Viewing commit history - Pulling from GitHub """ +import logging from typing import Dict, Optional, List from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import BaseModel @@ -16,6 +17,8 @@ from dependencies import get_current_user, AuthorizedAgentByName, OwnedAgentByName from services import git_service +logger = logging.getLogger(__name__) + router = APIRouter(prefix="/api/agents", tags=["git"]) @@ -349,47 +352,66 @@ async def initialize_github_sync( detail=f"Repository '{repo_full_name}' does not exist. Set create_repo=true to create it, or use an existing repository." ) - # Step 2: Initialize git in container (using git_service) - init_result = await git_service.initialize_git_in_container( + # Step 2: Reserve the working branch BEFORE touching the container. + # S7 Layer 0 (#382): goes through the single-entry helper so the + # remote probe + DB insert under the partial UNIQUE index happen + # atomically. If anything in the rest of this handler fails we + # roll the DB row back below so retries can claim a fresh branch. + instance_id, reserved_branch = await git_service.reserve_and_generate_instance_id( agent_name=agent_name, github_repo=repo_full_name, - github_pat=github_pat ) - if not init_result.success: - # Determine if this is a user error (400) or server error (500) - error_msg = init_result.error or "Unknown error" - # Repository not found during push = user configuration error - if "Repository not found" in error_msg or "not found" in error_msg.lower(): + try: + # Step 3: Initialize git in container using the reserved branch. + # `create_working_branch=False` tells the helper not to generate + # its own ID — the caller owns the reservation now (S7 Layer 0). + init_result = await git_service.initialize_git_in_container( + agent_name=agent_name, + github_repo=repo_full_name, + github_pat=github_pat, + create_working_branch=False, + working_branch=reserved_branch, + ) + + if not init_result.success: + # Determine if this is a user error (400) or server error (500) + error_msg = init_result.error or "Unknown error" + # Repository not found during push = user configuration error + if "Repository not found" in error_msg or "not found" in error_msg.lower(): + raise HTTPException( + status_code=400, + detail=f"Git initialization failed: {error_msg}. Verify the repository exists and you have push access." + ) + # Permission issues = user error + if "permission" in error_msg.lower() or "403" in error_msg: + raise HTTPException( + status_code=400, + detail=f"Git initialization failed: {error_msg}. Check that your GitHub PAT has push access to this repository." + ) + # Other errors could still be server issues raise HTTPException( status_code=400, - detail=f"Git initialization failed: {error_msg}. Verify the repository exists and you have push access." + detail=f"Git initialization failed: {error_msg}" ) - # Permission issues = user error - if "permission" in error_msg.lower() or "403" in error_msg: - raise HTTPException( - status_code=400, - detail=f"Git initialization failed: {error_msg}. Check that your GitHub PAT has push access to this repository." + except Exception: + # Release the reservation so a retry can grab a fresh branch. + try: + db.delete_git_config(agent_name) + except Exception as cleanup_exc: + logger.warning( + "Failed to roll back agent_git_config for %s after init " + "failure: %s", + agent_name, + cleanup_exc, ) - # Other errors could still be server issues - raise HTTPException( - status_code=400, - detail=f"Git initialization failed: {error_msg}" - ) - - # Step 3: Store configuration in database - instance_id = git_service.generate_instance_id() - config = await git_service.create_git_config_for_agent( - agent_name=agent_name, - github_repo=repo_full_name, - instance_id=instance_id - ) + raise return { "success": True, "message": "GitHub sync initialized successfully", "github_repo": repo_full_name, - "working_branch": init_result.working_branch, + "working_branch": reserved_branch, "instance_id": instance_id, "repo_url": f"https://github.com/{repo_full_name}" } diff --git a/src/backend/services/agent_service/crud.py b/src/backend/services/agent_service/crud.py index 61fb4d06f..df50953ce 100644 --- a/src/backend/services/agent_service/crud.py +++ b/src/backend/services/agent_service/crud.py @@ -221,9 +221,22 @@ async def create_agent_internal( # Log but don't block creation for transient network errors logger.warning(f"GitHub repo validation failed (non-blocking): {e}") - # Generate git sync instance ID and branch for Phase 7 - git_instance_id = git_service.generate_instance_id() - git_working_branch = git_service.generate_working_branch(config.name, git_instance_id) + # Generate git sync instance ID and branch for Phase 7. + # S7 Layer 0 (#382): reserve the working branch atomically — + # probes the remote with `git ls-remote` and inserts the DB + # row under the partial UNIQUE index so no two agents can end + # up bound to the same (repo, branch). The row is written + # here, before the container is created, so it must be rolled + # back if anything in the rest of the flow fails (see the + # `try: ... except: db.delete_git_config(...)` block below). + git_instance_id, git_working_branch = ( + await git_service.reserve_and_generate_instance_id( + agent_name=config.name, + github_repo=github_repo_for_agent, + source_branch=config.source_branch or "main", + source_mode=config.source_mode, + ) + ) elif config.template.startswith("local:"): # Local template - strip "local:" prefix template_name = config.template[6:] # Remove "local:" prefix @@ -607,21 +620,11 @@ async def create_agent_internal( except Exception as e: logger.warning(f"Failed to grant default permissions for {config.name}: {e}") - # Phase 7: Create git config for GitHub-native agents - if github_repo_for_agent: - try: - # In source mode, working_branch = source_branch (no separate working branch) - effective_working_branch = config.source_branch if config.source_mode else git_working_branch - db.create_git_config( - agent_name=config.name, - github_repo=github_repo_for_agent, - working_branch=effective_working_branch, - instance_id=git_instance_id, - source_branch=config.source_branch or "main", - source_mode=config.source_mode - ) - except Exception as e: - logger.warning(f"Failed to create git config for {config.name}: {e}") + # Phase 7: git config was already reserved and persisted via + # `reserve_and_generate_instance_id` earlier in this function + # (S7 Layer 0). No second db.create_git_config call here — that + # would either be a no-op (agent_name UNIQUE) or, worse, mask + # a Layer 2 conflict. # S4 (#383): Materialize persistent-state allowlist into the agent. # Runtime sync/reset paths read `.trinity/persistent-state.yaml`; @@ -645,6 +648,19 @@ async def create_agent_internal( return agent_status except Exception as e: + # S7 Layer 0 (#382): if anything after the reservation fails, + # roll back the agent_git_config row so the working branch is + # released and a retry can claim it fresh. + if github_repo_for_agent and git_instance_id: + try: + db.delete_git_config(config.name) + except Exception as cleanup_exc: + logger.warning( + "Failed to roll back agent_git_config for %s after " + "creation failure: %s", + config.name, + cleanup_exc, + ) logger.error(f"Failed to create agent {config.name}: {e}") raise HTTPException(status_code=500, detail="Failed to create agent. Please try again.") else: diff --git a/src/backend/services/git_service.py b/src/backend/services/git_service.py index 4111aaeec..de2bfaab9 100644 --- a/src/backend/services/git_service.py +++ b/src/backend/services/git_service.py @@ -7,15 +7,17 @@ - Managing git configuration in the database - Initializing git in agent containers """ +import asyncio import httpx import os import re +import sqlite3 import uuid import logging from dataclasses import dataclass from datetime import datetime from enum import Enum -from typing import Optional, Dict, Any, List +from typing import Optional, Dict, Any, List, Tuple from database import db, AgentGitConfig, GitSyncResult from services.docker_service import get_agent_container, execute_command_in_container @@ -131,8 +133,22 @@ def classify_conflict( return ConflictClass.UNKNOWN +# S7 Layer 0: how many times reserve_and_generate_instance_id retries on +# a remote/DB collision before giving up. 5 is generous — with a 32-bit +# UUID prefix the probability of a single collision is ~0 and the probability +# of five in a row is astronomically small, so this catches only real bugs +# (e.g. the caller feeding us a non-unique repo). +MAX_INSTANCE_ID_RETRIES = 5 + + def generate_instance_id() -> str: - """Generate a unique instance ID for an agent.""" + """Generate a unique instance ID for an agent. + + NOTE (S7 Layer 0): this returns a raw UUID prefix with no remote/DB + collision check. New call sites should use + ``reserve_and_generate_instance_id`` instead; this is kept only for + helpers that need the raw generator (e.g. inside the reserve helper). + """ return uuid.uuid4().hex[:8] @@ -230,6 +246,167 @@ async def _persistent_state_for(agent_name: str) -> list[str]: return [str(p) for p in patterns] +async def check_remote_branch_exists(github_repo: str, branch: str) -> bool: + """Return True if ``refs/heads/`` exists on the remote. + + Uses ``git ls-remote`` so the check does not require the GitHub REST API + or a specific auth mode — anything that can `git fetch` can also + `git ls-remote`. Returns False on network/command errors: the caller + treats that as "proceed with caution", since a stale "false" only costs + us an extra DB-insert collision which Layer 2 catches. + + S7 Layer 0 — part of the pre-flight for ``reserve_and_generate_instance_id``. + """ + # Prefer https://github.com/.git so the command works whether or + # not the backend has a PAT configured. Public repos answer ls-remote + # unauthenticated; private repos fall through to False and Layer 2 + # catches any duplicate insert. + remote_url = f"https://github.com/{github_repo}.git" + ref = f"refs/heads/{branch}" + + try: + proc = await asyncio.create_subprocess_exec( + "git", + "ls-remote", + "--heads", + "--exit-code", + remote_url, + ref, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + try: + stdout, _stderr = await asyncio.wait_for(proc.communicate(), timeout=15.0) + except asyncio.TimeoutError: + proc.kill() + await proc.wait() + logger.warning( + "git ls-remote timed out for %s %s — treating as 'not present'", + github_repo, + branch, + ) + return False + except FileNotFoundError: + logger.warning("git not installed on backend host; skipping remote branch check") + return False + except Exception as exc: # pragma: no cover — defensive + logger.warning( + "git ls-remote failed for %s %s: %s — treating as 'not present'", + github_repo, + branch, + exc, + ) + return False + + # --exit-code: 0 = ref found, 2 = not found. Anything else is an error + # we log and treat as "not present" (Layer 2 catches real duplicates). + if proc.returncode == 0: + return bool(stdout.strip()) + if proc.returncode == 2: + return False + logger.warning( + "git ls-remote %s %s exited %s — treating as 'not present'", + github_repo, + branch, + proc.returncode, + ) + return False + + +async def reserve_and_generate_instance_id( + agent_name: str, + github_repo: str, + source_branch: str = "main", + source_mode: bool = False, + sync_paths: Optional[List[str]] = None, +) -> Tuple[str, str]: + """Atomically reserve a fresh working branch for an agent. + + S7 Layer 0 — single entry point for generating an instance ID. Combines: + 1. UUID generation + 2. ``git ls-remote`` probe against the remote (Layer 1) + 3. DB insert into ``agent_git_config`` under the partial UNIQUE index + ``UNIQUE(github_repo, working_branch) WHERE source_mode = 0`` (Layer 2) + + Retries on either a remote hit or a DB IntegrityError up to + ``MAX_INSTANCE_ID_RETRIES`` times, then raises ``RuntimeError``. + + For ``source_mode=True`` the branch is the source branch (e.g. ``main``), + the remote probe is skipped (intentional shared-branch mode), and the DB + insert bypasses the partial UNIQUE index by design. + + Returns: + A ``(instance_id, working_branch)`` tuple. The DB row is already + persisted when this function returns. + + Raises: + RuntimeError: if ``MAX_INSTANCE_ID_RETRIES`` consecutive reservations + collide on either the remote or the DB. + """ + last_error: Optional[BaseException] = None + + for attempt in range(1, MAX_INSTANCE_ID_RETRIES + 1): + if source_mode: + # Source-mode agents share the source branch intentionally. + instance_id = generate_instance_id() + working_branch = source_branch + else: + instance_id = generate_instance_id() + working_branch = generate_working_branch(agent_name, instance_id) + + if await check_remote_branch_exists(github_repo, working_branch): + logger.warning( + "reserve_and_generate_instance_id: remote collision for %s " + "(attempt %d/%d)", + working_branch, + attempt, + MAX_INSTANCE_ID_RETRIES, + ) + continue + + try: + config = db.create_git_config( + agent_name=agent_name, + github_repo=github_repo, + working_branch=working_branch, + instance_id=instance_id, + sync_paths=sync_paths, + source_branch=source_branch, + source_mode=source_mode, + ) + except sqlite3.IntegrityError as exc: + last_error = exc + # The partial UNIQUE index on (github_repo, working_branch) WHERE + # source_mode = 0 fired — another agent already owns this branch. + # Retry with a fresh UUID. + logger.warning( + "reserve_and_generate_instance_id: DB collision for %s " + "(attempt %d/%d): %s", + working_branch, + attempt, + MAX_INSTANCE_ID_RETRIES, + exc, + ) + continue + + if config is None: + # create_git_config returns None on a plain agent_name UNIQUE + # violation — this is a different bug (agent already has config) + # and should not be silently retried. Surface immediately. + raise RuntimeError( + f"reserve_and_generate_instance_id: agent_git_config already " + f"exists for agent {agent_name!r}" + ) + + return instance_id, working_branch + + raise RuntimeError( + f"reserve_and_generate_instance_id: could not reserve a fresh working " + f"branch for {agent_name!r} in {github_repo!r} after " + f"{MAX_INSTANCE_ID_RETRIES} retries (last error: {last_error!r})" + ) + + async def create_git_config_for_agent( agent_name: str, github_repo: str, @@ -478,7 +655,8 @@ async def initialize_git_in_container( agent_name: str, github_repo: str, github_pat: str, - create_working_branch: bool = True + create_working_branch: bool = True, + working_branch: Optional[str] = None, ) -> GitInitResult: """ Initialize git in an agent container. @@ -490,13 +668,22 @@ async def initialize_git_in_container( 4. Configure remote 5. Create initial commit 6. Push to GitHub - 7. Create working branch (optional) + 7. Create working branch (optional; prefer the pre-reserved path) Args: agent_name: Name of the agent container github_repo: Full repo name (e.g., "owner/repo") github_pat: GitHub PAT for authentication - create_working_branch: Whether to create a working branch + create_working_branch: DEPRECATED (S7 Layer 0 / #382). When True the + helper generates an instance ID internally, bypassing the + `reserve_and_generate_instance_id` collision check. New callers + MUST pre-reserve via `reserve_and_generate_instance_id` and pass + `create_working_branch=False, working_branch=` instead. + working_branch: Pre-reserved working branch name (e.g. + ``trinity//``). Required when + ``create_working_branch=False``. Mutually exclusive with + internal generation — when set, this function just checks out / + pushes that branch. Returns: GitInitResult with status and branch info @@ -614,9 +801,37 @@ async def initialize_git_in_container( error=f"Git command failed: {cmd}\nOutput: {output}" ) - # Step 4: Create working branch (optional) - working_branch = None - if create_working_branch: + # Step 4: Create (or check out) the working branch. + # S7 Layer 0 (#382): prefer the pre-reserved path — callers pass + # `working_branch=` and `create_working_branch=False`. The + # legacy `create_working_branch=True` path falls back to an internal + # `generate_instance_id()` call and is deprecated; it's kept so older + # callers don't break, but emits a warning on every use. + if working_branch is not None: + branch_commands = [ + f"git checkout -b {working_branch}", + f"git push -u origin {working_branch}", + ] + for cmd in branch_commands: + result = await execute_command_in_container( + container_name=container_name, + command=f'bash -c "cd {git_dir} && {cmd}"', + timeout=60, + ) + if result.get("exit_code", 0) != 0: + logger.warning( + "Failed to create pre-reserved working branch %s: %s", + working_branch, + result.get("output", ""), + ) + elif create_working_branch: + # Deprecated path — no caller should hit this after S7 rolls out. + logger.warning( + "initialize_git_in_container(create_working_branch=True) is " + "deprecated (S7 / #382). Pre-reserve via " + "reserve_and_generate_instance_id and pass working_branch " + "explicitly." + ) instance_id = generate_instance_id() working_branch = generate_working_branch(agent_name, instance_id) diff --git a/tests/git-sync/conftest.py b/tests/git-sync/conftest.py index 800357410..fdb0afac3 100644 --- a/tests/git-sync/conftest.py +++ b/tests/git-sync/conftest.py @@ -1,7 +1,22 @@ -"""Local conftest — isolates tests/git-sync/ from the parent tests/conftest.py. +""" +Local conftest for tests/git-sync/. -The parent conftest authenticates against a running backend (401 against -localhost:8000) which is noise for these pure-function tests. By defining -this local conftest, pytest's root-conftest discovery is satisfied at this -level and we avoid pulling in the parent's fixtures. +The top-level tests/conftest.py wires up HTTP fixtures and auto-logs in +against a live backend, which these unit tests don't need. We override the +backend-dependent fixtures with no-ops so pytest can collect and run the +S7 unit tests in complete isolation. """ + +import pytest + + +@pytest.fixture(scope="session") +def api_client(): + """Override the live-backend api_client fixture.""" + yield None + + +@pytest.fixture(autouse=True) +def cleanup_after_test(): + """Override the autouse cleanup_after_test fixture that hits the backend.""" + yield diff --git a/tests/git-sync/test_p5_branch_ownership.sh b/tests/git-sync/test_p5_branch_ownership.sh new file mode 100755 index 000000000..afd95232a --- /dev/null +++ b/tests/git-sync/test_p5_branch_ownership.sh @@ -0,0 +1,104 @@ +#!/usr/bin/env bash +# S7 Layer 3 Tier-1 test — verify that --force-with-lease catches the +# silent-clobber bug from issue #382. +# +# Two agents clone a shared bare repo, land on the same working branch, +# write conflicting workspace state, then both try to push with +# --force-with-lease using the SHA each observed at fetch time. The +# second push MUST be rejected with "stale info" / non-zero exit. +# +# References: +# - /tmp/trinity-repro/p5_silent_clobber.sh (demonstrates the bug) +# - /tmp/trinity-repro/p5_fix_verified.sh (demonstrates the fix) +# - docker/base-image/agent_server/routers/git.py (the site being fixed) +# +# Usage: bash tests/git-sync/test_p5_branch_ownership.sh +# Exit: 0 on success, 1 on failure. + +set -u + +ROOT="$(mktemp -d -t trinity-s7-p5-XXXXXX)" +trap 'rm -rf "$ROOT"' EXIT +cd "$ROOT" + +echo "=== S7 Layer 3: --force-with-lease rejects silent clobber ===" +echo " work dir: $ROOT" +echo + +git init --bare -q -b main bare.git +git clone -q bare.git seed +( + cd seed + git config user.email seed@t.local + git config user.name Seed + echo init > README.md + git add . + git commit -qm init + git push -q -u origin main + git checkout -qb trinity/alpaca/a702560e + git push -q -u origin trinity/alpaca/a702560e +) +rm -rf seed + +for a in agent-a agent-b; do + git clone -q bare.git "$a" + ( + cd "$a" + git config user.email "$a@t.local" + git config user.name "$a" + git checkout -q trinity/alpaca/a702560e + ) +done + +# Each agent observes the remote SHA at fetch time; that becomes the lease. +LEASE_A=$(cd agent-a && git rev-parse origin/trinity/alpaca/a702560e) +LEASE_B=$(cd agent-b && git rev-parse origin/trinity/alpaca/a702560e) +echo "[agent-a] observed lease SHA: $LEASE_A" +echo "[agent-b] observed lease SHA: $LEASE_B" + +# Each agent mutates the branch locally. +(cd agent-a && echo ALPHA > workspace.txt && git add . && git commit -qm "Agent A state") +(cd agent-b && echo BETA > workspace.txt && git add . && git commit -qm "Agent B state") + +echo +echo "[push] agent-a pushes first with lease $LEASE_A" +cd agent-a +git push --force-with-lease=trinity/alpaca/a702560e:"$LEASE_A" origin trinity/alpaca/a702560e 2>&1 | sed 's/^/ agent-a: /' +EXIT_A=${PIPESTATUS[0]} +echo " agent-a exit: $EXIT_A" +cd .. + +echo +echo "[push] agent-b pushes second with STALE lease $LEASE_B — must be rejected" +cd agent-b +PUSH_OUT=$(git push --force-with-lease=trinity/alpaca/a702560e:"$LEASE_B" origin trinity/alpaca/a702560e 2>&1) +EXIT_B=$? +echo "$PUSH_OUT" | sed 's/^/ agent-b: /' +echo " agent-b exit: $EXIT_B" +cd .. + +echo +echo "=== RESULT ===" +FAIL=0 +if [ "$EXIT_A" != "0" ]; then + echo "FAIL: agent-a (lease matched) should have succeeded, got exit $EXIT_A" + FAIL=1 +fi +if [ "$EXIT_B" = "0" ]; then + echo "FAIL: agent-b (stale lease) should have been REJECTED, got exit 0" + FAIL=1 +fi +if ! echo "$PUSH_OUT" | grep -qiE 'stale info|rejected|non-fast-forward'; then + echo "FAIL: agent-b rejection message did not mention 'stale info' / 'rejected'" + FAIL=1 +fi + +if [ "$FAIL" = "0" ]; then + echo "OK: --force-with-lease rejects the losing push with stale-ref error." + echo " Agent-b now KNOWS it lost, so Trinity can surface a collision" + echo " entry to the operator queue." + exit 0 +else + echo "The S7 Layer-3 invariant is violated." + exit 1 +fi diff --git a/tests/git-sync/test_s7_reserve_instance_id.py b/tests/git-sync/test_s7_reserve_instance_id.py new file mode 100644 index 000000000..d11bb40e7 --- /dev/null +++ b/tests/git-sync/test_s7_reserve_instance_id.py @@ -0,0 +1,601 @@ +""" +S7 Layer 0/1/2 unit tests for branch-ownership enforcement (issue #382). + +Covers: + (i) `reserve_and_generate_instance_id` retries when `ls-remote` shows a + collision. + (ii) After the configured max retries the helper raises. + (iii) The partial UNIQUE index on `agent_git_config(github_repo, + working_branch) WHERE source_mode = 0` rejects duplicate bindings + for working-branch agents and accepts duplicates for source-mode + agents (intentional — source-mode agents share e.g. `main`). + (iv) The migration pre-flight surfaces existing duplicate bindings and + refuses to install the UNIQUE index until operators resolve them. + +The tests use an in-memory SQLite database with just the pieces of schema +they need — no live backend required. `ls-remote` is patched so no network +is hit. +""" + +from __future__ import annotations + +import asyncio +import os +import sqlite3 +import sys +import types +from datetime import datetime, timezone +from pathlib import Path +from unittest.mock import patch + +import pytest + + +# --------------------------------------------------------------------------- +# Path / module stubs so we can import the backend's git_service cleanly +# without importing database.py (which wires up the full DB on import). +# --------------------------------------------------------------------------- +_REPO_ROOT = Path(__file__).resolve().parents[2] +_BACKEND = _REPO_ROOT / "src" / "backend" +if str(_BACKEND) not in sys.path: + sys.path.insert(0, str(_BACKEND)) + + +# tests/utils shadows src/backend/utils, so stub the helpers module we +# need from the backend side. +if "utils.helpers" not in sys.modules: + _helpers = types.ModuleType("utils.helpers") + _helpers.utc_now = lambda: datetime.now(timezone.utc) # type: ignore[attr-defined] + _helpers.utc_now_iso = lambda: datetime.now(timezone.utc).isoformat() # type: ignore[attr-defined] + _helpers.to_utc_iso = lambda v: str(v) # type: ignore[attr-defined] + _helpers.parse_iso_timestamp = ( # type: ignore[attr-defined] + lambda s: datetime.fromisoformat(s.rstrip("Z")) + ) + sys.modules["utils.helpers"] = _helpers + + +def _ensure_module_stubs() -> None: + """Provide lightweight stubs for modules git_service.py imports at import time.""" + if "database" not in sys.modules: + database = types.ModuleType("database") + + class AgentGitConfig: # minimal container, only fields the tests touch + def __init__(self, **kwargs): + self.__dict__.update(kwargs) + + class GitSyncResult: + def __init__(self, **kwargs): + self.__dict__.update(kwargs) + + class _Db: # replaced by the test fixtures when they need DB calls + def create_git_config(self, *args, **kwargs): + raise NotImplementedError("test did not install a db stub") + + database.AgentGitConfig = AgentGitConfig + database.GitSyncResult = GitSyncResult + database.db = _Db() + sys.modules["database"] = database + + if "services.docker_service" not in sys.modules: + docker_service = types.ModuleType("services.docker_service") + docker_service.get_agent_container = lambda *a, **kw: None + + async def _exec(*args, **kwargs): + return {"exit_code": 0, "output": ""} + + docker_service.execute_command_in_container = _exec + # Make "services" a package rooted at src/backend/services so that + # `from services import git_service` picks up the real module. + if "services" not in sys.modules: + services_pkg = types.ModuleType("services") + services_pkg.__path__ = [str(_BACKEND / "services")] # type: ignore[attr-defined] + sys.modules["services"] = services_pkg + sys.modules["services.docker_service"] = docker_service + + +_ensure_module_stubs() + +# Clear any cached git_service to pick up edits between the test file and +# the implementation. +sys.modules.pop("services.git_service", None) +from services import git_service # noqa: E402 (after sys.path tweak) + + +# --------------------------------------------------------------------------- +# In-memory SQLite helpers +# --------------------------------------------------------------------------- +GIT_CONFIG_DDL = """ + CREATE TABLE agent_git_config ( + id TEXT PRIMARY KEY, + agent_name TEXT UNIQUE NOT NULL, + github_repo TEXT NOT NULL, + working_branch TEXT NOT NULL, + instance_id TEXT NOT NULL, + source_branch TEXT DEFAULT 'main', + source_mode INTEGER DEFAULT 0, + created_at TEXT NOT NULL, + last_sync_at TEXT, + last_commit_sha TEXT, + sync_enabled INTEGER DEFAULT 1, + sync_paths TEXT, + github_pat_encrypted TEXT + ) +""" + +# This is the index under test — S7 Layer 2. +PARTIAL_UNIQUE_DDL = ( + "CREATE UNIQUE INDEX idx_git_config_repo_branch_unique " + "ON agent_git_config(github_repo, working_branch) " + "WHERE source_mode = 0" +) + + +def _insert_row( + cursor: sqlite3.Cursor, + *, + agent_name: str, + github_repo: str, + working_branch: str, + source_mode: int, + instance_id: str = "deadbeef", +) -> None: + cursor.execute( + """ + INSERT INTO agent_git_config + (id, agent_name, github_repo, working_branch, instance_id, + source_branch, source_mode, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + f"cfg-{agent_name}", + agent_name, + github_repo, + working_branch, + instance_id, + "main", + source_mode, + datetime.now(timezone.utc).isoformat(), + ), + ) + + +@pytest.fixture +def db_conn(): + conn = sqlite3.connect(":memory:") + conn.execute(GIT_CONFIG_DDL) + conn.commit() + yield conn + conn.close() + + +# --------------------------------------------------------------------------- +# Layer 2 — partial UNIQUE index behaviour +# --------------------------------------------------------------------------- +class TestPartialUniqueConstraint: + def test_rejects_duplicate_working_branches_for_source_mode_0(self, db_conn): + """Two agents cannot bind to the same (repo, branch) when source_mode=0.""" + db_conn.execute(PARTIAL_UNIQUE_DDL) + cur = db_conn.cursor() + + _insert_row( + cur, + agent_name="alpaca-a", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + db_conn.commit() + + with pytest.raises(sqlite3.IntegrityError): + _insert_row( + cur, + agent_name="alpaca-b", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + db_conn.commit() + + def test_allows_duplicate_branches_for_source_mode_1(self, db_conn): + """Source-mode agents intentionally share a branch (e.g. main). Partial index excludes them.""" + db_conn.execute(PARTIAL_UNIQUE_DDL) + cur = db_conn.cursor() + + _insert_row( + cur, + agent_name="reader-a", + github_repo="org/docs", + working_branch="main", + source_mode=1, + ) + _insert_row( + cur, + agent_name="reader-b", + github_repo="org/docs", + working_branch="main", + source_mode=1, + ) + db_conn.commit() + + rows = cur.execute( + "SELECT agent_name FROM agent_git_config WHERE source_mode = 1" + ).fetchall() + assert sorted(r[0] for r in rows) == ["reader-a", "reader-b"] + + def test_allows_same_branch_across_different_repos(self, db_conn): + """The uniqueness scope is per-repo: same branch name, different repo is fine.""" + db_conn.execute(PARTIAL_UNIQUE_DDL) + cur = db_conn.cursor() + + _insert_row( + cur, + agent_name="alpaca-a", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + _insert_row( + cur, + agent_name="alpaca-b", + github_repo="org/other", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + db_conn.commit() + + +# --------------------------------------------------------------------------- +# Layer 0 — reserve_and_generate_instance_id +# --------------------------------------------------------------------------- +def _run(coro): + loop = asyncio.new_event_loop() + try: + return loop.run_until_complete(coro) + finally: + loop.close() + + +class _FakeDb: + """Minimal db stub: records each create_git_config call.""" + + def __init__(self): + self.created = [] + + def create_git_config( + self, + agent_name, + github_repo, + working_branch, + instance_id, + sync_paths=None, + source_branch="main", + source_mode=False, + ): + self.created.append( + { + "agent_name": agent_name, + "github_repo": github_repo, + "working_branch": working_branch, + "instance_id": instance_id, + "source_branch": source_branch, + "source_mode": source_mode, + } + ) + return types.SimpleNamespace( + agent_name=agent_name, + github_repo=github_repo, + working_branch=working_branch, + instance_id=instance_id, + ) + + +class TestReserveAndGenerateInstanceId: + def test_helper_exists(self): + assert hasattr( + git_service, "reserve_and_generate_instance_id" + ), "S7 Layer 0 helper reserve_and_generate_instance_id must exist" + assert hasattr( + git_service, "check_remote_branch_exists" + ), "S7 Layer 0 helper check_remote_branch_exists must exist" + + def test_returns_fresh_branch_when_remote_is_clean(self, monkeypatch): + """Happy path: remote reports no collision, helper returns on first try.""" + + async def _no_collision(github_repo, branch): + return False + + fake_db = _FakeDb() + monkeypatch.setattr(git_service, "check_remote_branch_exists", _no_collision) + monkeypatch.setattr(git_service, "db", fake_db, raising=False) + + instance_id, working_branch = _run( + git_service.reserve_and_generate_instance_id( + agent_name="alpaca", + github_repo="org/alpaca", + ) + ) + + assert working_branch == f"trinity/alpaca/{instance_id}" + assert len(instance_id) == 8 + assert len(fake_db.created) == 1 + assert fake_db.created[0]["working_branch"] == working_branch + + def test_retries_on_remote_collision(self, monkeypatch): + """When ls-remote reports a collision the helper picks a new UUID and retries.""" + calls = {"n": 0} + + async def _collide_twice_then_clear(github_repo, branch): + calls["n"] += 1 + return calls["n"] <= 2 # collide on 1st and 2nd probe, clear on 3rd + + fake_db = _FakeDb() + monkeypatch.setattr( + git_service, "check_remote_branch_exists", _collide_twice_then_clear + ) + monkeypatch.setattr(git_service, "db", fake_db, raising=False) + + instance_id, working_branch = _run( + git_service.reserve_and_generate_instance_id( + agent_name="alpaca", + github_repo="org/alpaca", + ) + ) + + assert calls["n"] == 3 + assert working_branch == f"trinity/alpaca/{instance_id}" + assert len(fake_db.created) == 1 + + def test_raises_after_max_retries(self, monkeypatch): + """If every probe collides the helper gives up with a descriptive error.""" + + async def _always_collide(github_repo, branch): + return True + + fake_db = _FakeDb() + monkeypatch.setattr(git_service, "check_remote_branch_exists", _always_collide) + monkeypatch.setattr(git_service, "db", fake_db, raising=False) + + with pytest.raises(RuntimeError) as exc_info: + _run( + git_service.reserve_and_generate_instance_id( + agent_name="alpaca", + github_repo="org/alpaca", + ) + ) + + msg = str(exc_info.value).lower() + assert "collision" in msg or "retr" in msg or "reserve" in msg + assert fake_db.created == [] # never inserted a row + + def test_db_insert_failure_triggers_retry(self, monkeypatch): + """If the DB unique constraint rejects the row we try a fresh ID.""" + + async def _no_collision(github_repo, branch): + return False + + calls = {"inserts": 0} + + class _FlakyDb: + def __init__(self): + self.created = [] + + def create_git_config(self, *args, **kwargs): + calls["inserts"] += 1 + if calls["inserts"] == 1: + raise sqlite3.IntegrityError( + "UNIQUE constraint failed: idx_git_config_repo_branch_unique" + ) + self.created.append(kwargs) + return types.SimpleNamespace(**kwargs) + + flaky = _FlakyDb() + monkeypatch.setattr(git_service, "check_remote_branch_exists", _no_collision) + monkeypatch.setattr(git_service, "db", flaky, raising=False) + + instance_id, working_branch = _run( + git_service.reserve_and_generate_instance_id( + agent_name="alpaca", + github_repo="org/alpaca", + ) + ) + + assert calls["inserts"] == 2 + assert flaky.created and flaky.created[0]["working_branch"] == working_branch + + +# --------------------------------------------------------------------------- +# Migration pre-flight +# --------------------------------------------------------------------------- +class TestMigrationPreflight: + def test_detects_existing_duplicates(self, db_conn): + """ + The migration must surface duplicate bindings (same repo+branch with + source_mode=0) before trying to create the partial UNIQUE index, so + operators can fix them first. + """ + # Import the migration helper lazily — the test runs before the + # helper exists at TDD-red time, which is the desired failure mode. + sys.modules.pop("db.migrations", None) + try: + from db.migrations import ( # type: ignore[import] + _find_duplicate_working_branches, + ) + except ImportError as exc: # pragma: no cover — TDD red path + pytest.fail( + f"S7 migration helper _find_duplicate_working_branches is missing: {exc}" + ) + + cur = db_conn.cursor() + _insert_row( + cur, + agent_name="alpaca-a", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + _insert_row( + cur, + agent_name="alpaca-b", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + # A third, unrelated binding should NOT appear in the result. + _insert_row( + cur, + agent_name="other", + github_repo="org/other", + working_branch="trinity/other/deadbeef", + source_mode=0, + ) + db_conn.commit() + + duplicates = _find_duplicate_working_branches(cur) + + assert len(duplicates) == 1, f"expected one duplicate group, got {duplicates!r}" + group = duplicates[0] + assert group["github_repo"] == "org/alpaca" + assert group["working_branch"] == "trinity/alpaca/a702560e" + assert sorted(group["agent_names"]) == ["alpaca-a", "alpaca-b"] + + def test_no_duplicates_returns_empty_list(self, db_conn): + sys.modules.pop("db.migrations", None) + try: + from db.migrations import ( # type: ignore[import] + _find_duplicate_working_branches, + ) + except ImportError as exc: # pragma: no cover — TDD red path + pytest.fail( + f"S7 migration helper _find_duplicate_working_branches is missing: {exc}" + ) + + cur = db_conn.cursor() + _insert_row( + cur, + agent_name="alpaca-a", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + _insert_row( + cur, + agent_name="reader-a", + github_repo="org/docs", + working_branch="main", + source_mode=1, + ) + _insert_row( + cur, + agent_name="reader-b", + github_repo="org/docs", + working_branch="main", + source_mode=1, + ) + db_conn.commit() + + assert _find_duplicate_working_branches(cur) == [] + + def test_migration_aborts_when_duplicates_exist(self, db_conn): + """ + End-to-end: running the S7 migration against a DB with pre-existing + duplicates must raise with a message that references the offending + rows, instead of silently deleting them. + """ + sys.modules.pop("db.migrations", None) + try: + from db.migrations import ( # type: ignore[import] + _migrate_agent_git_config_branch_ownership, + ) + except ImportError as exc: # pragma: no cover — TDD red path + pytest.fail( + f"S7 migration function _migrate_agent_git_config_branch_ownership is missing: {exc}" + ) + + cur = db_conn.cursor() + _insert_row( + cur, + agent_name="alpaca-a", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + _insert_row( + cur, + agent_name="alpaca-b", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + db_conn.commit() + + with pytest.raises(Exception) as exc_info: + _migrate_agent_git_config_branch_ownership(cur, db_conn) + + msg = str(exc_info.value) + assert "alpaca-a" in msg and "alpaca-b" in msg + # The index must NOT exist after an aborted migration. + indexes = [ + r[0] + for r in cur.execute( + "SELECT name FROM sqlite_master WHERE type='index' " + "AND tbl_name='agent_git_config'" + ).fetchall() + ] + assert "idx_git_config_repo_branch_unique" not in indexes + + def test_migration_installs_index_on_clean_db(self, db_conn): + sys.modules.pop("db.migrations", None) + try: + from db.migrations import ( # type: ignore[import] + _migrate_agent_git_config_branch_ownership, + ) + except ImportError as exc: # pragma: no cover — TDD red path + pytest.fail( + f"S7 migration function _migrate_agent_git_config_branch_ownership is missing: {exc}" + ) + + cur = db_conn.cursor() + _insert_row( + cur, + agent_name="alpaca-a", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + _insert_row( + cur, + agent_name="reader-a", + github_repo="org/docs", + working_branch="main", + source_mode=1, + ) + _insert_row( + cur, + agent_name="reader-b", + github_repo="org/docs", + working_branch="main", + source_mode=1, + ) + db_conn.commit() + + _migrate_agent_git_config_branch_ownership(cur, db_conn) + + indexes = [ + r[0] + for r in cur.execute( + "SELECT name FROM sqlite_master WHERE type='index' " + "AND tbl_name='agent_git_config'" + ).fetchall() + ] + assert "idx_git_config_repo_branch_unique" in indexes + + # Now the constraint must bite for source_mode=0. + with pytest.raises(sqlite3.IntegrityError): + _insert_row( + cur, + agent_name="alpaca-b", + github_repo="org/alpaca", + working_branch="trinity/alpaca/a702560e", + source_mode=0, + ) + db_conn.commit()