From 45430e32ca77717fa6771875b0d75a08ee964fd8 Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Sat, 18 Apr 2026 14:31:07 +0100 Subject: [PATCH 1/7] test(git): add S7 branch-ownership tests for #382 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two complementary tests written red-first per TDD: - tests/git-sync/test_p5_branch_ownership.sh — pure-git Tier-1 repro. Two agents land on the same working branch, observe the remote SHA at fetch time, then race to push with --force-with-lease. The losing side must exit non-zero with "stale info". Adapted from /tmp/trinity-repro/p5_silent_clobber.sh and p5_fix_verified.sh. - tests/git-sync/test_s7_reserve_instance_id.py — pytest suite. Covers the new reserve_and_generate_instance_id helper (retries, max-retries exhaustion, DB rollback), the partial UNIQUE index on agent_git_config(github_repo, working_branch) WHERE source_mode = 0, and the migration pre-flight that refuses to install the index when existing duplicates are present. Local conftest overrides the top-level tests/conftest.py backend-login fixtures so the unit tests run without a live backend. Refs #382 --- tests/git-sync/conftest.py | 25 +- tests/git-sync/test_p5_branch_ownership.sh | 104 +++ tests/git-sync/test_s7_reserve_instance_id.py | 601 ++++++++++++++++++ 3 files changed, 725 insertions(+), 5 deletions(-) create mode 100755 tests/git-sync/test_p5_branch_ownership.sh create mode 100644 tests/git-sync/test_s7_reserve_instance_id.py 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() From 18541d606bb52e04819844d879efc421573adb81 Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Sat, 18 Apr 2026 14:31:15 +0100 Subject: [PATCH 2/7] feat(db): add S7 Layer 2 partial UNIQUE index on agent_git_config (#382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds UNIQUE(github_repo, working_branch) WHERE source_mode = 0 to agent_git_config so two working-branch agents can no longer be bound to the same (repo, branch) pair. Source-mode agents intentionally share branches (every reader pointing at main), so the partial predicate excludes them. Migration pre-flight: - _find_duplicate_working_branches scans for existing duplicates. - _migrate_agent_git_config_branch_ownership refuses to create the index when duplicates are found, printing and logging each offending row so the operator can rebind one of the colliding agents to a fresh working branch before re-running. Never auto-deletes — that would mask the bug this constraint catches. Fresh installs get the index from schema.py; existing installs pick it up via the new migration step. Refs #382 --- src/backend/db/migrations.py | 104 +++++++++++++++++++++++++++++++++++ src/backend/db/schema.py | 7 +++ 2 files changed, 111 insertions(+) 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)", From ec22b386f3b34f8853ad401b14040879fa738c81 Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Sat, 18 Apr 2026 14:31:33 +0100 Subject: [PATCH 3/7] feat(git): consolidate instance-id generation behind S7 Layer 0 helper (#382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the three ad-hoc generate_instance_id() call sites with a single reserve_and_generate_instance_id(agent_name, github_repo, ...) that atomically: 1. Generates a UUID and builds the working branch name. 2. Probes the remote via `git ls-remote --heads --exit-code` so a name that already exists on GitHub is never re-used (Layer 1). 3. Inserts the agent_git_config row under the partial UNIQUE index introduced by the companion migration (Layer 2). 4. Retries on either collision up to MAX_INSTANCE_ID_RETRIES (5), then raises RuntimeError rather than silently picking a shared branch. Call site changes: - src/backend/services/agent_service/crud.py: reserve BEFORE container creation; drop the duplicate db.create_git_config call that previously ran after container boot; roll back the reservation on any later failure so retries can claim a fresh branch. - src/backend/routers/git.py::initialize_github_sync: reserve BEFORE initialize_git_in_container; pass the reserved branch in and use create_working_branch=False so the helper doesn't generate its own; roll back on failure. - src/backend/services/git_service.py::initialize_git_in_container: add `working_branch` kwarg for the pre-reserved path; deprecate create_working_branch=True with a warning log (kept for legacy callers until they're all migrated). The plain generate_instance_id / generate_working_branch helpers are kept but documented as internal — new code must go through reserve_and_generate_instance_id. Refs #382 --- src/backend/routers/git.py | 78 ++++--- src/backend/services/agent_service/crud.py | 52 +++-- src/backend/services/git_service.py | 231 ++++++++++++++++++++- 3 files changed, 307 insertions(+), 54 deletions(-) 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) From 83157843add36c2ea2e4a89c88310d91d549fb08 Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Sat, 18 Apr 2026 14:31:46 +0100 Subject: [PATCH 4/7] feat(agent-server): push with --force-with-lease and surface collisions (#382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S7 Layer 3 — defense in depth for the force-push path. - Replace `git push --force` with `git push --force-with-lease=:`. The expected SHA is the remote value we observed at the last successful fetch; if another instance wrote to the branch since, the lease is stale and the push is rejected cleanly with "stale info" instead of silently clobbering the peer's state (2026-04-17 alpaca incident). - After every successful `git fetch`, persist the observed remote SHA to `~/.trinity/last-remote-sha/`. The push path reads this as the lease. Failure to persist is logged but never raised — worst case we fall back to the unparameterized `--force-with-lease`, which uses the remote-tracking ref as the expected-sha. - When the lease is rejected we append a structured `alert` entry to `~/.trinity/operator-queue.json` so the backend's operator_queue sync service picks it up on the next poll and surfaces the collision in the Operating Room. The agent learns it lost the race instead of believing its push succeeded. Returns HTTP 409 with `X-Conflict-Type: branch_ownership_collision` on lease rejection so the frontend can distinguish it from the existing `push_rejected` / `merge_conflict` cases. Refs #382 --- docker/base-image/agent_server/routers/git.py | 194 +++++++++++++++++- 1 file changed, 190 insertions(+), 4 deletions(-) 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( From 3e85cd858320a6cabb99fcfd180fa8aeb0228bf3 Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Sun, 19 Apr 2026 01:45:20 +0100 Subject: [PATCH 5/7] docs(feature-flows): document S7 Layers 0/2 in github-repo-initialization (#382) Reflects the Layer 0 reservation helper and Layer 2 partial UNIQUE constraint introduced by PR #396: - New `reserve_and_generate_instance_id` (atomic UUID + ls-remote probe + DB insert under partial UNIQUE, retries 5x). - Three legacy `generate_instance_id()` call sites consolidated to reserve-before-create with rollback. - Partial UNIQUE on `agent_git_config(github_repo, working_branch) WHERE source_mode = 0` + migration with duplicate pre-flight that refuses to install on existing duplicates. - Sequence diagram split into Phase 4a (reserve loop) / 4b (container init); Phase 5 retitled. Layer 3 push-time guard documented separately in github-sync.md. Refs #382 #381 --- .../github-repo-initialization.md | 210 +++++++++++++++--- 1 file changed, 174 insertions(+), 36 deletions(-) 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: From af0b22902c876bcc19136701b04ded8dc66c223b Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Sun, 19 Apr 2026 01:47:43 +0100 Subject: [PATCH 6/7] docs(feature-flows): document S7 Layer 3 push-time guard in github-sync (#382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reflects the agent-server changes in commit e2d8402: - `git push --force` → `git push --force-with-lease=:` - Last-observed remote SHA persisted to `~/.trinity/last-remote-sha/` on every successful fetch and read as the lease on push. - Lease rejection returns 409 with `X-Conflict-Type: branch_ownership_collision` and appends a structured alert to `~/.trinity/operator-queue.json` so the Operating Room surfaces the collision instead of the agent believing its push succeeded. - 2026-04-17 alpaca-vybe-live incident cited as motivation; regression at tests/git-sync/test_p5_branch_ownership.sh. Layers 0/2 (reservation + partial UNIQUE) documented in github-repo-initialization.md. Refs #382 #381 --- docs/memory/feature-flows/github-sync.md | 104 ++++++++++++++++++++--- 1 file changed, 93 insertions(+), 11 deletions(-) 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. | From dd32d2c578593154059895f0c5e96bcf6e8f7d28 Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Sun, 19 Apr 2026 01:48:37 +0100 Subject: [PATCH 7/7] =?UTF-8?q?docs(requirements):=20add=20=C2=A711.3=20Br?= =?UTF-8?q?anch=20Ownership=20Enforcement=20(S7,=20#382)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documents the four-layer S7 enforcement landed in PR #396 — reservation helper + partial UNIQUE + force-with-lease + collision alert — alongside the existing GitHub Sync / Repo Initialization sections. Refs #382 #381 --- docs/memory/requirements.md | 10 ++++++++++ 1 file changed, 10 insertions(+) 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