From ee8929840571a262410ce1ecfe88d1a44cdce59a Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Fri, 25 Sep 2026 15:52:53 +0300 Subject: [PATCH 1/8] feat(git-sync): the container pulls origin on its own (trinity-enterprise#703) Invariant G3: human and fleet work reaches the agent within a bound. Nothing inside the container ever pulled; the 2026-09-24 audit found agents up to 31 commits behind. - agent server: a pull loop beside the push loop, gated per cycle on the owner's pull_sync_enabled (read live, GIT_SYNC_PULL fallback), interval GIT_SYNC_PULL_INTERVAL_SECONDS (defaults to the push one). Under _REPO_LOCK, never while an execution runs (checked again right before the tree is touched). Fast-forward, or rebase aborted on conflict. Uncommitted edits are stashed explicitly and a pull that collides with them is undone - not --autostash, which strands them in the stash while reporting success. - pull_sync_enabled + last_pull_at/_status/behind_after_pull on both migration tracks; backfill on only where auto-sync is on; new github agents get it at creation (source mode included), GIT_SYNC_PULL env derived from the DB flag alone at recreate. - GET/PUT /api/agents/{name}/git/pull-sync; sync-health persists the pull outcome (bounded); Settings -> Git sync gains the toggle. Stacked on #3020 (+#3015-#3018): merges after it. Related to abilityai/trinity-enterprise#703 Co-Authored-By: Claude Opus 5.5 (1M context) --- docker/base-image/agent_server/auto_sync.py | 119 ++++++- docker/base-image/agent_server/routers/git.py | 147 ++++++++ docs/memory/architecture/agent-lifecycle.md | 1 + docs/memory/architecture/api-endpoints.md | 1 + docs/memory/feature-flows/git-sync-health.md | 39 +++ docs/memory/requirements/github.md | 24 ++ src/backend/database.py | 6 + src/backend/db/migrations.py | 36 ++ src/backend/db/schedules/git_config.py | 21 ++ src/backend/db/schema.py | 4 + src/backend/db/sync_state.py | 11 + src/backend/db/tables.py | 4 + src/backend/db_models.py | 1 + .../migrations/versions/0076_pull_sync.py | 43 +++ src/backend/models.py | 5 + src/backend/routers/git.py | 28 ++ src/backend/services/agent_service/crud.py | 13 + .../services/agent_service/lifecycle.py | 9 + src/backend/services/sync_health_service.py | 18 + .../src/components/GitSyncSettingsPanel.vue | 44 ++- src/frontend/src/stores/agents.js | 18 + .../tests/unit/gitSyncSettingsPanel.spec.js | 29 +- tests/registry.json | 11 + ...test_1484_create_agent_characterization.py | 15 + tests/unit/test_ent109_git_env_seam.py | 16 +- tests/unit/test_ent703_pull_cycle.py | 316 ++++++++++++++++++ tests/unit/test_sync_health_service.py | 37 ++ 27 files changed, 996 insertions(+), 20 deletions(-) create mode 100644 src/backend/migrations/versions/0076_pull_sync.py create mode 100644 tests/unit/test_ent703_pull_cycle.py diff --git a/docker/base-image/agent_server/auto_sync.py b/docker/base-image/agent_server/auto_sync.py index 81158be11e..c6578445be 100644 --- a/docker/base-image/agent_server/auto_sync.py +++ b/docker/base-image/agent_server/auto_sync.py @@ -61,43 +61,50 @@ def should_start_loop() -> bool: return should_run_auto_sync() or _platform_coords() is not None -async def resolve_auto_sync_enabled(client: httpx.AsyncClient) -> bool: - """This cycle's gate: the owner's `auto_sync_enabled`, asked live (#3010). +async def _resolve_flag(client: httpx.AsyncClient, path: str, field: str, + fallback: bool, label: str) -> bool: + """One live read of a per-agent git flag, with the agent's own key. - 200 → the flag. - - 404 "Git not configured" → False: nothing is bound to sync to. + - 404 "Git not configured" → False: nothing is bound to sync with. - anything else (backend down, 5xx, auth refusal, a uniform 404) → the env fallback, which the last recreate derived from the same flag. Failing to the last known value keeps a platform blip from arming a disabled loop or silencing an enabled one. """ - global _last_resolved - value = should_run_auto_sync() + value = fallback coords = _platform_coords() if coords is not None: backend_url, mcp_key, agent_name = coords try: resp = await client.get( - f"{backend_url}/api/agents/{agent_name}/git/auto-sync", + f"{backend_url}/api/agents/{agent_name}/git/{path}", headers={"Authorization": f"Bearer {mcp_key}"}, timeout=_FLAG_TIMEOUT, ) if resp.status_code == 200: - value = bool(resp.json().get("auto_sync_enabled")) + value = bool(resp.json().get(field)) elif resp.status_code == 404 and _detail(resp) == "Git not configured": value = False else: logger.warning( - "auto-sync: flag read returned %s; using env fallback (%s)", - resp.status_code, value, + "%s: flag read returned %s; using env fallback (%s)", + label, resp.status_code, value, ) except (httpx.HTTPError, ValueError) as exc: - logger.warning("auto-sync: flag read failed (%s); using env fallback (%s)", - type(exc).__name__, value) - _last_resolved = value + logger.warning("%s: flag read failed (%s); using env fallback (%s)", + label, type(exc).__name__, value) return value +async def resolve_auto_sync_enabled(client: httpx.AsyncClient) -> bool: + """This cycle's push gate: the owner's `auto_sync_enabled`, asked live (#3010).""" + global _last_resolved + _last_resolved = await _resolve_flag( + client, "auto-sync", "auto_sync_enabled", should_run_auto_sync(), "auto-sync") + return _last_resolved + + def _detail(resp: httpx.Response) -> Optional[str]: try: body = resp.json() @@ -173,13 +180,90 @@ async def run_one_cycle(client: httpx.AsyncClient, home: Path, run_once) -> Opti return result +# --------------------------------------------------------------------------- +# trinity-enterprise#703 — the pull cycle (invariant G3: human and fleet work +# reaches the agent within a bound). A second loop beside the push loop, gated +# per cycle on the owner's `pull_sync_enabled`; the cycle itself is +# `routers/git.py::_run_pull_once` (never under a running execution, never +# discards local work). +# --------------------------------------------------------------------------- + +_last_pull_resolved: Optional[bool] = None + + +def should_run_pull() -> bool: + """The pull cycle's env fallback (`GIT_SYNC_PULL`), derived from the DB flag + at every recreate.""" + return os.getenv("GIT_SYNC_PULL", "").lower() == "true" + + +async def resolve_pull_sync_enabled(client: httpx.AsyncClient) -> bool: + global _last_pull_resolved + _last_pull_resolved = await _resolve_flag( + client, "pull-sync", "pull_sync_enabled", should_run_pull(), "pull") + return _last_pull_resolved + + +def current_pull_sync_enabled() -> bool: + return _last_pull_resolved if _last_pull_resolved is not None else should_run_pull() + + +def get_pull_interval_seconds() -> int: + """`GIT_SYNC_PULL_INTERVAL_SECONDS`, defaulting to the push interval.""" + raw = os.getenv("GIT_SYNC_PULL_INTERVAL_SECONDS") + if not raw: + return get_interval_seconds() + try: + value = int(raw) + return value if value > 0 else get_interval_seconds() + except ValueError: + logger.warning("GIT_SYNC_PULL_INTERVAL_SECONDS=%r is not an int; using the push interval", raw) + return get_interval_seconds() + + +async def run_one_pull_cycle(client: httpx.AsyncClient, home: Path, run_once) -> Optional[dict]: + """One pull tick: skip without a repo or with the owner's pull flag off, + else run the pull in a worker thread (it runs git children).""" + if not (home / ".git").exists(): + return None + if not await resolve_pull_sync_enabled(client): + logger.debug("pull: off (pull_sync_enabled=0), skipping cycle") + return None + result = await asyncio.to_thread(run_once, home) + logger.info("pull: %s", result.get("status")) + return result + + +async def run_pull_loop( + home_dir: Optional[Path] = None, interval_seconds: Optional[int] = None +) -> None: + """Background pull loop. Swallows every exception to keep running.""" + from .routers.git import _run_pull_once # lazy: avoids circular import + + home = home_dir or _HOME_DIR + interval = interval_seconds if interval_seconds is not None else get_pull_interval_seconds() + logger.info("pull loop started (interval=%ss, home=%s)", interval, home) + async with httpx.AsyncClient() as client: + try: + await resolve_pull_sync_enabled(client) + except Exception: # noqa: BLE001 — loop must never die + logger.exception("pull: initial flag read raised unexpectedly") + await asyncio.sleep(interval) + while True: + try: + await run_one_pull_cycle(client, home, _run_pull_once) + except Exception: # noqa: BLE001 — loop must never die + logger.exception("pull cycle raised unexpectedly") + await asyncio.sleep(interval) + + def schedule_auto_sync_if_enabled(app) -> None: """Attach the loop's startup handler when auto-sync is on or can be turned on live (#3010); each cycle then checks the owner's flag.""" - if not should_start_loop(): + if not (should_start_loop() or should_run_pull()): logger.info( - "auto-sync unavailable (no GIT_SYNC_AUTO and no platform " - "credentials to read the per-agent flag)" + "auto-sync unavailable (no GIT_SYNC_AUTO / GIT_SYNC_PULL and no " + "platform credentials to read the per-agent flags)" ) return @@ -187,8 +271,9 @@ def schedule_auto_sync_if_enabled(app) -> None: @app.on_event("startup") async def _start_auto_sync() -> None: - task = asyncio.create_task(run_auto_sync_loop()) - task_ref.append(task) + task_ref.append(asyncio.create_task(run_auto_sync_loop())) + # trinity-enterprise#703: the pull loop runs beside it, gated per cycle. + task_ref.append(asyncio.create_task(run_pull_loop())) @app.on_event("shutdown") async def _stop_auto_sync() -> None: diff --git a/docker/base-image/agent_server/routers/git.py b/docker/base-image/agent_server/routers/git.py index 9bee9c7cef..b6f7f7d196 100644 --- a/docker/base-image/agent_server/routers/git.py +++ b/docker/base-image/agent_server/routers/git.py @@ -118,6 +118,10 @@ def _conflict_response( "last_lock_recovery": None, # #2742: the boot reap's own record of a wedge "last_successful_push_at": None, # #3011: last cycle whose push landed "behind_after_fetch": None, # #3011: commits origin/ had that we lacked + "last_pull_at": None, # trinity-enterprise#703: the container's pull cycle + "last_pull_status": "never", + "last_pull_error": None, + "behind_after_pull": None, } @@ -823,6 +827,146 @@ def _rebase_onto_remote(home_dir: Path, branch: str) -> Optional[str]: return _summarize_git_error(rebase.stderr or rebase.stdout or "rebase failed") +def _executions_in_flight() -> Optional[int]: + """How many executions are running on this agent, or None if unknown. + Indirection so the pull cycle's execution gate is unit-testable.""" + try: + from ..services.process_registry import get_process_registry + return len(get_process_registry().list_running()) + except Exception: # noqa: BLE001 — unknown is treated as busy by the caller + logger.debug("pull: could not read the process registry", exc_info=True) + return None + + +def _record_pull(home_dir: Path, status: str, *, error: Optional[str] = None, + behind: Optional[int] = None) -> Dict: + """Persist one pull outcome WITHOUT touching the push fields — the push's + `consecutive_failures` / `last_sync_*` stay the push's (trinity-enterprise#703).""" + _patch_sync_state(home_dir, { + "last_pull_at": datetime.now(timezone.utc).isoformat(), + "last_pull_status": status, + "last_pull_error": error, + "behind_after_pull": behind, + }) + result = {"status": status} + if error: + result["error"] = error + return result + + +def _integrate_remote(home_dir: Path, branch: str, ahead: int) -> Optional[str]: + """Bring `origin/` into the checked-out branch without ever losing + local work (trinity-enterprise#703). Returns None, or the error recorded. + + Uncommitted tracked edits are stashed EXPLICITLY — not `--autostash`: when + incoming commits touch the same files, autostash's re-apply conflicts and + leaves the edits only in the stash while the pull reports success, so the + files silently lose them. Here a conflicting re-apply is undone: back to the + pre-pull HEAD, where the stash applies cleanly to its own base, and the + failure is recorded. Untracked files are never stashed; one an incoming + commit would overwrite fails the fast-forward/rebase instead. + """ + def git(*args, timeout=60): + return run_registered(["git", *args], cwd=str(home_dir), timeout=timeout) + + pre_head = git("rev-parse", "HEAD", timeout=10).stdout.strip() + dirty = git("status", "--porcelain", "--untracked-files=no", timeout=10).stdout.strip() + stashed = False + if dirty: + push = git("stash", "push", "-m", "trinity-pull (ent#703)", timeout=30) + if push.returncode != 0: + return _summarize_git_error(push.stderr or push.stdout or "stash failed") + stashed = True + + if ahead == 0: + step = git("merge", "--ff-only", f"origin/{branch}") + err = None if step.returncode == 0 else _summarize_git_error( + step.stderr or step.stdout or "fast-forward failed") + else: + err = _rebase_onto_remote(home_dir, branch) # aborts itself on failure + + if err: + if stashed: # nothing moved (ff refused / rebase aborted): put the edits back + git("stash", "pop", timeout=30) + return err + + if stashed: + pop = git("stash", "pop", timeout=30) + if pop.returncode != 0: + # The edits collide with what came in. Undo the pull — the stash is + # still there — and re-apply on the base it was taken from. + git("reset", "--hard", pre_head, timeout=30) + back = git("stash", "pop", timeout=30) + if back.returncode != 0: + return ("local edits conflict with incoming changes on " + f"{branch}; they are kept in `git stash`") + return f"local edits conflict with incoming changes on {branch}" + return None + + +def _run_pull_once(home_dir: Path) -> Dict: + """One pull cycle (trinity-enterprise#703, invariant G3): bring origin's + commits for the checked-out branch to the agent. + + Source-mode agents are checked out on their source branch and working-branch + agents on their working branch, so the checked-out branch is the one to pull. + + - Never while an execution runs (a rebase under a running turn changes the + files it is reading) — checked before the fetch and again right before the + tree is touched; an unreadable registry counts as busy. + - Serialised with the push cycle and the operator git endpoints on + `_REPO_LOCK`; a busy repo skips quietly. + - `_integrate_remote`: a fast-forward when nothing is committed locally, a + rebase (aborted on conflict) when something is; uncommitted edits are + stashed explicitly and re-applied, and a pull whose incoming changes + collide with them is undone. Local work is never discarded; a failure is + recorded, not resolved. + """ + # The lock first: every write to sync-state.json happens under it, because + # the push cycle read-modify-writes the same file from another thread. + if not _REPO_LOCK.acquire(blocking=False): + return {"status": "skipped", "reason": "repo_busy"} + try: + running = _executions_in_flight() + if running is None or running > 0: + return _record_pull(home_dir, "skipped", error=( + "execution in flight" if running else "execution state unknown")) + branch = run_registered( + ["git", "symbolic-ref", "--short", "-q", "HEAD"], + cwd=str(home_dir), timeout=10, + ).stdout.strip() + if not branch: + return _record_pull(home_dir, "failed", error="detached HEAD: nothing to pull onto") + fetch = run_registered( + ["git", "fetch", "origin", branch], cwd=str(home_dir), timeout=120, + ) + if fetch.returncode != 0: + if _is_missing_remote_ref(fetch.stderr): + return _record_pull(home_dir, "success", behind=0) # not on origin yet + return _record_pull(home_dir, "failed", error=_summarize_git_error( + fetch.stderr or fetch.stdout or "fetch failed")) + ahead, behind = _compute_ahead_behind(home_dir, branch) + if behind == 0: + return _record_pull(home_dir, "success", behind=0) + running = _executions_in_flight() + if running is None or running > 0: + return _record_pull(home_dir, "skipped", behind=behind, error=( + "execution in flight" if running else "execution state unknown")) + err = _integrate_remote(home_dir, branch, ahead) + if err: + return _record_pull(home_dir, "failed", behind=behind, error=err) + _, after = _compute_ahead_behind(home_dir, branch) + logger.info("pull: %s brought in %s commit(s) on %s", home_dir, behind, branch) + return _record_pull(home_dir, "success", behind=after) + except subprocess.CalledProcessError as exc: + return _record_pull(home_dir, "failed", error=_summarize_git_error( + exc.stderr or exc.stdout or str(exc))) + except Exception as exc: # noqa: BLE001 — the loop must never die + return _record_pull(home_dir, "failed", error=_summarize_git_error(str(exc))) + finally: + _REPO_LOCK.release() + + def _run_auto_sync_once(home_dir: Path) -> Dict: """One auto-sync cycle: reap stale lock litter, measure, stage, commit if dirty, fetch + rebase onto the remote branch, push, maybe consolidate .git. @@ -1437,6 +1581,9 @@ def _compute_git_status(home_dir: Path) -> Dict: # flag as last read, the same source `GET .../git/auto-sync` returns. from ..auto_sync import current_auto_sync_enabled response["auto_sync_enabled"] = current_auto_sync_enabled() + # trinity-enterprise#703: the pull gate the pull loop is running with. + from ..auto_sync import current_pull_sync_enabled + response["pull_sync_enabled"] = current_pull_sync_enabled() # #2742: a currently-stuck lock is REPORTED, never removed. This is the # "tell the truth about state" half — a stale lock does not fail # `git status` (rc=0, empty stderr), so before this the read could not diff --git a/docs/memory/architecture/agent-lifecycle.md b/docs/memory/architecture/agent-lifecycle.md index 89fb261ebf..54b3fc86a3 100644 --- a/docs/memory/architecture/agent-lifecycle.md +++ b/docs/memory/architecture/agent-lifecycle.md @@ -170,6 +170,7 @@ An agent's Claude Code marketplace-plugin selection is a first-class, **committe ### Git Sync Health (#389/#390) +**Pull cycle (trinity-enterprise#703):** a second loop beside the push loop, `auto_sync.run_pull_loop` → `routers/git.py::_run_pull_once`. It is gated per cycle on the owner's `pull_sync_enabled`, read live from `GET .../git/pull-sync`, with `GIT_SYNC_PULL` as the fallback; the interval is `GIT_SYNC_PULL_INTERVAL_SECONDS`, defaulting to the push interval. It never runs while an execution is in flight (process registry, checked under `_REPO_LOCK` and again before the tree is touched). `_integrate_remote` stashes uncommitted edits explicitly and undoes a pull whose incoming changes collide with them, instead of autostash, which would strand them in the stash. Outcome goes to `sync-state.json` (`last_pull_*`), persisted on `agent_sync_state`. **Agent side:** 15-min `auto_sync` heartbeat loop in the agent server (each cycle gated live on the owner's `auto_sync_enabled`, read with the agent's own key from `GET .../git/auto-sync`, `GIT_SYNC_AUTO` as the unreachable-platform fallback — #3010; default-on for non-source-mode GitHub-template agents) stages/commits in-container changes, fetches and rebases onto `origin/` before a lease-protected push (a conflict aborts and records `diverged: …`; a source-mode agent on the default branch refuses — #3011), and writes the outcome to `.trinity/sync-state.json` (S1a; atomic tmp+`os.replace` write). **Maintenance ownership (#1595):** git's own auto-gc can NEVER complete in an agent container — it detaches to PID 1 and the #817 orphan sweep SIGKILLs it every time (44 GB / 97%-garbage repos observed) — so the base image disables it (`gc.auto=0`, `gc.autoDetach=false`, `maintenance.auto=false` in `/etc/gitconfig`; also in `git_service` setup for older images) and the auto-sync loop is the **single maintenance owner** for the agent's home repo (sub-repos cloned into the workspace get no maintenance — documented blind spot). The cycle runs in a worker thread (`asyncio.to_thread` — a long repack no longer starves `/health`/heartbeats), every subprocess is sweep-registered via `utils/registered_run.py` (`ProcessRegistry.add_transient_pid`, process-group timeout kill), and a non-blocking repo `threading.Lock` serializes it against the mutating git endpoints (`sync`/`pull`/reset → 409 `agent_busy` on contention; busy cycle skips). Each cycle reaps stale lock litter age-gated (`gc.pid`/`index.lock` >1h, `tmp_pack_*` > repack budget; `startup.sh` reaps unconditionally at container start — the stale-`index.lock` 12-day-freeze class). **#2742 — the status read stops racing the agent's own git:** `GET /api/git/status` (the surface the backend's 60 s poll drives) used to run ~8 bare `subprocess.run` git children on the event loop, including `git status --porcelain`, which takes `.git/index.lock` on EVERY invocation to refresh the index — so the platform took that lock ~2×/min in every workspace, outside `_REPO_LOCK`, and a sweep-killed child orphaned a 0-byte lock that silently failed every later git write. The read now uses `git --no-optional-locks status --porcelain -z` (scoped to this one call site — the commit paths still want the writeback), routes every child through `run_registered`, and is computed in ONE `to_thread` worker behind a loop-level single-flight so the poller, the UI git panel and the MCP tool coalesce onto one computation and one `git fetch origin` (`computed_at` stamps the resulting bounded staleness). A present `index.lock` is **reported, never deleted** — 0 bytes is the signature of a *live* `git add` for ~100 % of its life and `st_mtime` never advances, so staleness is decided by two-point inode stability (same inode unchanged across ≥3 ticks / ≥15 min), gitdir-resolved to cover worktrees and submodules, `lstat`-only (no repo lock, hence no new 409 `agent_busy`) and wrapped in its own `except OSError` so the observer can never darken the feed it feeds. Recovery stays where it is provably race-free — the boot reap, which now *announces* what it cleared and records it as `sync_state.last_lock_recovery`. **Bloat controls (#1596/#1595):** after a successful push the loop runs a consolidating **repack** (`git repack -A -d -l --unpack-unreachable=1.hour.ago` + `git gc --prune=1.hour.ago` — 1h grace so concurrent Claude-run git ops are never corrupted; `pack.threads=1` + `pack.windowMemory=128m` bound RSS) when packs ≥ `GIT_MAINTENANCE_PACK_THRESHOLD` (20) OR loose objects ≥ `GIT_MAINTENANCE_LOOSE_THRESHOLD` (6700 — with auto-gc off, garbage accumulates loose); guarded by a free-disk preflight (skip < 1.1× pack bytes), an exponential failure backoff (1h→24h, persisted in sync-state), and an env-tunable budget (`GIT_MAINTENANCE_TIMEOUT_SECONDS`, default 1800, read at call time). Non-destructive: does NOT rewrite reachable history. It measures `.git` size (`du -sb`) plus `git count-objects -v` pack/loose counts into sync-state on **every** terminal path (failing repos must not go dark). The fleet-wide default `.gitignore` (`git_service._GITIGNORE_PATTERNS`, **merged into each agent on sync and at creation** — #2069, readiness-gated) also excludes bulk data/deps/caches (`node_modules/`, `.venv/`, `__pycache__/`, `*.sqlite`/`*.db`, …) so churny non-source files aren't auto-committed. **Creation-time seed (#2069):** the list was applied only on Push/init, never at creation, so an agent whose 15-min auto-sync loop is on from birth (the `GIT_SYNC_AUTO` set — non-source/fork `github:` agents, ephemeral ghosts included) auto-committed `.trinity/` state + root `.env`/`.mcp.json` into its user-owned repo before any Push could migrate the list. `git_service.spawn_gitignore_merge_after_clone` fires a fire-and-forget, agent-`/health`-readiness-gated (server launches at `startup.sh:517` after ALL git setup, ~900s before the first cycle) merge — reusing `_build_gitignore_merge_command` (no fourth pattern list), merge-only (PREVENT: stops `git add -A` staging the untracked generated creds), gated on the shared `_git_auto_sync_baked` ENV predicate, idempotent (#953). Wired at creation (`_materialize_agent_files`) and at start/recreate (`start_agent_internal`, gated on the DB `auto_sync_enabled` flag — fleet remediation for existing leakers). **`.trinity/` is ignored contents-only (#2070):** `.trinity/*` plus a `!` re-include per path in `_TRINITY_AUTHORED_PATHS` (`pre-check`, `post-check`, `pre-snapshot`, `setup.sh`, `persistent-processes.allow`, `brain-orb/`, `pipelines/`, `plugins.yaml` #1704) — the files a TEMPLATE commits and the platform reads back. The wholesale `.trinity/` exclusion made every Push's `git rm --cached` untrack them and push the deletion, guarded only by a hardcoded two-entry pathspec that was wrong three times (brain-orb ent#76, `setup.sh`, then `pre-check` #2070); the symptom is silent because only the index is touched, surfacing one re-clone/recreate later as an agent with no hook. Now the default under `.trinity/` is *ignored*, so a new runtime file needs no action, while authored content is tracked by construction — and the `rm --cached` exemptions are DERIVED from the same constant so the two cannot drift. The merge also **removes** the superseded exact line `.trinity/` (git never descends into a dir-form exclusion, so the re-includes under it would be inert on every pre-#2070 agent). **Precedence — TWO managed regions (#2529):** the merge is a normalize-and-rebuild, not an append, because a canonical rule appended *below* an agent's negation silently reverses it and the sweep then untracks the file — and a canonical rule hoisted *above* a platform negation silently reverses that one instead, which is why there are two managed regions and not one. `[defaults block][user region, original order][protected floor]`: the agent's `!.env.example` / `!.claude/settings.json` beats a default without having to be the file's last line, while the seven credential patterns (`.env`, `.env.*`, `.mcp.json`, `credentials.json`, `*.pem`, `*.key`, `.ssh/` — membership decided against `credential_paths.py`, not against the constant's comment headings, which is how `.ssh/` was missed on the first pass) with their two canonical negations and `.trinity/*` with its eight derived `!` re-includes sit BELOW the user region and cannot be overridden (a single hoisted block would flip every currently-inert `!.env` in the fleet live in one unattended Push, and would let a user `*.sh` beat `!.trinity/setup.sh` — ent#76/#1704 reintroduced, and quietly, since the rm-cached pathspec still exempts the path). Idempotent by content: a second run computes byte-identical output and the file is only written when it differs. **Residual, reported not fixed:** 23 canonical patterns are dir-form, and git does not descend into an excluded directory, so a negation beneath one is inert at *any* position — contents-form conversion was costed and rejected (it would force git to stat `node_modules/`-sized trees on every `status`), so the Push instead reports it via `shadowed_negations`, alongside `removed_paths` and `unignored_paths` on the API response, the MCP result, the toast, the commit message (best-effort — `git rm --cached` only stages) and a durable `gitignore_untracked` operator-queue entry — all five gated on `GitignoreSweep.changed_tracking` (removed OR unignored), because the rebuild changes what is in the repo in BOTH directions and the addition is already in the remote's history. Maintenance bounds *garbage*, not *history* — unbounded history growth remains the deferred opt-in squash / geometric-repack follow-up. **Backend side** (details in [git-sync-health.md](../feature-flows/git-sync-health.md)): `SyncHealthService` polls git-enabled agents every 60s (#2742: from the `synchealth:leader` lease-holder only, so `--workers 2` runs one poll not two — **fail-open**, because an observability feed must not go dark when Redis is; the cadence is `SYNC_HEALTH_POLL_INTERVAL_SECONDS`, default unchanged at 60 s. One poller instead of two halves the rate `consecutive_failures` climbs, so `sync_failing` now fires at ~3 min rather than ~90 s), upserts `agent_sync_state` (`consecutive_failures` ++ on fail / reset on success; `ahead_working`/`behind_working` expose working-branch divergence, P6; `git_dir_bytes` #1596 + `pack_count`/`loose_objects`/`maintenance_failures` #1595 carry the repo-health curve — all agent-supplied ints coerced at the boundary, the `/health clone_status` convention), emits `sync_failing` operator-queue entries at ≥3 failures (S1) and edge-triggered `git_bloat` entries when `git_dir_bytes` crosses `GIT_DIR_ALERT_BYTES` (default 10 GiB) or maintenance fails 3× running (#1595 — the failure class was previously silent until the disk filled). Powers the dashboard sync dot, `GET /api/agents/sync-health` (now incl. `git_dir_bytes`), and `GET /api/fleet/sync-audit` — whose `duplicate_binding` flag marks agents sharing a `(github_repo, working_branch)` pair (§P5 silent-clobber setup) (S6, #390). diff --git a/docs/memory/architecture/api-endpoints.md b/docs/memory/architecture/api-endpoints.md index 7046459db8..d996e84e8d 100644 --- a/docs/memory/architecture/api-endpoints.md +++ b/docs/memory/architecture/api-endpoints.md @@ -143,6 +143,7 @@ The per-agent VoIP config + voice-picker UI lives in the agent Settings/Sharing |--------|------|-------------| | GET/PUT/DELETE | `/api/agents/{name}/github-pat` | PAT config status / set per-agent PAT (validated, encrypted) / clear (revert to global) | | GET/PUT | `/api/agents/{name}/git/auto-sync` | Per-agent 15-min auto-sync heartbeat flag | +| GET/PUT | `/api/agents/{name}/git/pull-sync` | Per-agent pull-cycle flag, read live by the agent (trinity-enterprise#703) | | GET/PUT | `/api/agents/{name}/git/freeze-schedules-if-failing` | Freeze-on-sync-failure flag | | GET | `/api/agents/{name}/git/sync-state` | Persisted sync-state row | | POST | `/api/agents/{name}/git/reset-to-main-preserve-state` | Recovery reset — see [Git Sync Health](agent-lifecycle.md#git-sync-health-389390) | diff --git a/docs/memory/feature-flows/git-sync-health.md b/docs/memory/feature-flows/git-sync-health.md index e9db131277..3528ac0995 100644 --- a/docs/memory/feature-flows/git-sync-health.md +++ b/docs/memory/feature-flows/git-sync-health.md @@ -355,6 +355,45 @@ so it reconciles (all under `_REPO_LOCK`, every child via `run_registered`): lacked, before the rebase) and `last_successful_push_at` (stamped on success) in `sync-state.json`, for the divergence-age work (trinity-enterprise#706). +### 1d. Pull cycle — origin reaches the agent (trinity-enterprise#703) + +Invariant **G3**: human and fleet work reaches the agent within a bound. Before +this, nothing inside the container ever pulled. The only pulls were an +operator's, the fleet orchestrator's daily sweep, or a marketplace +SessionStart hook, and the 2026-09-24 audit found agents up to 31 commits behind. + +- **Loop:** `auto_sync.run_pull_loop`, beside the push loop, every + `GIT_SYNC_PULL_INTERVAL_SECONDS` (defaults to the push interval, 900 s). +- **Gate:** each cycle reads the owner's `pull_sync_enabled` live + (`GET .../git/pull-sync`, agent key), with `GIT_SYNC_PULL` as the fallback, + the #3010 pattern. +- **Where it's on:** + - new `github:` agents: on at creation, source-mode included + - existing agents: on only where `auto_sync_enabled` was already on + - everyone else: off until toggled in Settings → Git sync +- **Cycle** (`_run_pull_once`, under `_REPO_LOCK`): + - Skipped while any execution is running, checked again right before the + tree is touched; an unreadable registry counts as busy. + - Fetches the **checked-out** branch: source-mode agents sit on their source + branch, working-branch agents on theirs. + - Nothing committed locally → a fast-forward. A local commit → a rebase, + aborted on conflict (`diverged: …`). +- **Uncommitted edits** (`_integrate_remote`) are stashed explicitly and + re-applied. `--autostash` is deliberately not used: when the incoming commits + touch the same files, its re-apply conflicts, leaves the edits *only in the + stash*, and still reports success. Instead a colliding pull is **undone**: + back to the pre-pull HEAD, where the stash applies cleanly. The result is + recorded as `local edits conflict with incoming changes on `. +- **Recorded:** + - `sync-state.json`: `last_pull_at`, `last_pull_status` + (`success`/`failed`/`skipped`), `last_pull_error`, `behind_after_pull`. + These are written through `_patch_sync_state`, so the push's + `consecutive_failures` / `last_sync_*` are never touched. + - The poller persists them on `agent_sync_state` (status bounded to the + vocabulary, counter coerced). +- **Not here:** merging the source branch into a working branch on a longer + interval (noted in the issue); the hooks-vs-heartbeat owner (ent#708). + ### 2. Backend poller ``` diff --git a/docs/memory/requirements/github.md b/docs/memory/requirements/github.md index 083cda6b0b..8b943c5f51 100644 --- a/docs/memory/requirements/github.md +++ b/docs/memory/requirements/github.md @@ -853,3 +853,27 @@ - **GitHub Issue**: abilityai/trinity-enterprise#705 --- + +### 11.18 The Container Pulls Origin on Its Own (trinity-enterprise#703) + +- **Status**: 🚧 In review. Stacked on #3020; merges after it. +- **Invariant G3:** human and fleet work reaches the agent within a bound. +- **Pull cycle** in the agent server beside the push cycle: + - on its interval: fetch, then fast-forward or rebase + - uncommitted edits stashed and re-applied explicitly + - never discards local work: a conflict is aborted or undone, and recorded + - never runs while an execution is in flight +- **Flag:** per-agent `pull_sync_enabled` (both migration tracks), read live + each cycle, with `GIT_SYNC_PULL` as the fallback; interval + `GIT_SYNC_PULL_INTERVAL_SECONDS` (defaults to the push interval). + - on for new `github:` agents, source-mode included + - on for existing agents only where auto-sync is already on + - off otherwise; toggle in Settings → Git sync +- **Observability:** `last_pull_at`, `last_pull_status`, `behind_after_pull` + in `sync-state.json`, persisted on `agent_sync_state`. A failed pull never + counts toward the push's `consecutive_failures`. +- **Edition**: open-core (operator ruling 2026-09-25). +- **Flow**: `docs/memory/feature-flows/git-sync-health.md` §1d +- **GitHub Issue**: abilityai/trinity-enterprise#703 + +--- diff --git a/src/backend/database.py b/src/backend/database.py index 13a1ed087d..83e205858d 100644 --- a/src/backend/database.py +++ b/src/backend/database.py @@ -2033,6 +2033,12 @@ def set_freeze_schedules_if_sync_failing(self, agent_name: str, enabled: bool): def get_git_auto_sync_enabled(self, agent_name: str): return self._schedule_ops.get_git_auto_sync_enabled(agent_name) + def set_git_pull_sync_enabled(self, agent_name: str, enabled: bool): + return self._schedule_ops.set_git_pull_sync_enabled(agent_name, enabled) + + def get_git_pull_sync_enabled(self, agent_name: str): + return self._schedule_ops.get_git_pull_sync_enabled(agent_name) + def get_all_git_auto_sync_enabled(self, agent_names=None): return self._schedule_ops.get_all_git_auto_sync_enabled(agent_names) diff --git a/src/backend/db/migrations.py b/src/backend/db/migrations.py index 86f27ad096..8232b6c408 100644 --- a/src/backend/db/migrations.py +++ b/src/backend/db/migrations.py @@ -4700,6 +4700,41 @@ def _migrate_auto_sync_enabled_backfill(cursor, conn): conn.commit() +def _migrate_pull_sync(cursor, conn): + """The container's pull cycle (trinity-enterprise#703). + + * `agent_git_config.pull_sync_enabled` — the per-agent switch the agent's + pull loop reads live each cycle (the #3010 one-writer discipline). + * `agent_sync_state.last_pull_at / last_pull_status / behind_after_pull` — + the pull cycle's own outcome, persisted by the sync-health poller. + + Backfill (operator ruling 2026-09-25): on only where auto-sync is already + on, so no agent that is not already writing to git starts rebasing its + working tree on upgrade; everyone else is off until toggled. New `github:` + agents get it at creation. Runs once (schema_migrations). + + Mirrored by the Alembic revision 0076_pull_sync. + """ + _safe_add_column( + cursor, "agent_git_config", "pull_sync_enabled", + "ALTER TABLE agent_git_config ADD COLUMN pull_sync_enabled INTEGER DEFAULT 0", + ) + for column, ddl in ( + ("last_pull_at", "TEXT"), + ("last_pull_status", "TEXT"), + ("behind_after_pull", "INTEGER"), + ): + _safe_add_column( + cursor, "agent_sync_state", column, + f"ALTER TABLE agent_sync_state ADD COLUMN {column} {ddl}", + ) + cursor.execute( + "UPDATE agent_git_config SET pull_sync_enabled = 1 " + "WHERE COALESCE(auto_sync_enabled, 0) = 1" + ) + conn.commit() + + MIGRATIONS = [ ("agent_sharing", _migrate_agent_sharing_table), ("schedule_executions_observability", _migrate_schedule_executions_observability), @@ -4845,4 +4880,5 @@ def _migrate_auto_sync_enabled_backfill(cursor, conn): ("operator_queue_sync_state", _migrate_operator_queue_sync_state), ("role_readiness_rollout_seed", _migrate_role_readiness_rollout_seed), ("auto_sync_enabled_backfill", _migrate_auto_sync_enabled_backfill), + ("pull_sync", _migrate_pull_sync), ] diff --git a/src/backend/db/schedules/git_config.py b/src/backend/db/schedules/git_config.py index 0ac282c7f0..b4150f5783 100644 --- a/src/backend/db/schedules/git_config.py +++ b/src/backend/db/schedules/git_config.py @@ -47,6 +47,8 @@ def _row_to_git_config(row) -> AgentGitConfig: if "auto_sync_enabled" in row_keys else False, freeze_schedules_if_sync_failing=bool(row["freeze_schedules_if_sync_failing"]) if "freeze_schedules_if_sync_failing" in row_keys else False, + pull_sync_enabled=bool(row["pull_sync_enabled"]) # trinity-enterprise#703 + if "pull_sync_enabled" in row_keys and row["pull_sync_enabled"] is not None else False, ) # ========================================================================= @@ -179,6 +181,25 @@ def set_freeze_schedules_if_sync_failing(self, agent_name: str, enabled: bool) - ) return result.rowcount > 0 + def set_git_pull_sync_enabled(self, agent_name: str, enabled: bool) -> bool: + """trinity-enterprise#703: toggle the container's pull cycle.""" + with get_engine().begin() as conn: + result = conn.execute( + update(agent_git_config) + .where(agent_git_config.c.agent_name == agent_name) + .values(pull_sync_enabled=1 if enabled else 0) + ) + return result.rowcount > 0 + + def get_git_pull_sync_enabled(self, agent_name: str) -> bool: + """trinity-enterprise#703: read the pull flag. False if config missing.""" + stmt = select(agent_git_config.c.pull_sync_enabled).where( + agent_git_config.c.agent_name == agent_name + ) + with get_engine().connect() as conn: + row = conn.execute(stmt).first() + return bool(row[0]) if row else False + def get_git_auto_sync_enabled(self, agent_name: str) -> bool: """#389: read the auto-sync flag. False if config missing.""" stmt = select(agent_git_config.c.auto_sync_enabled).where( diff --git a/src/backend/db/schema.py b/src/backend/db/schema.py index a0c2871ca6..8c74ca0deb 100644 --- a/src/backend/db/schema.py +++ b/src/backend/db/schema.py @@ -1146,6 +1146,7 @@ github_pat_encrypted TEXT, auto_sync_enabled INTEGER DEFAULT 0, freeze_schedules_if_sync_failing INTEGER DEFAULT 0, + pull_sync_enabled INTEGER DEFAULT 0, -- trinity-enterprise#703 FOREIGN KEY (agent_name) REFERENCES agent_ownership(agent_name) ) """, @@ -1170,6 +1171,9 @@ pack_count INTEGER, loose_objects INTEGER, maintenance_failures INTEGER DEFAULT 0, + last_pull_at TEXT, -- trinity-enterprise#703: the container's pull cycle + last_pull_status TEXT, + behind_after_pull INTEGER, last_check_at TEXT, updated_at TEXT NOT NULL, FOREIGN KEY (agent_name) REFERENCES agent_ownership(agent_name) diff --git a/src/backend/db/sync_state.py b/src/backend/db/sync_state.py index 2d9d2089d5..0d3d3e0c15 100644 --- a/src/backend/db/sync_state.py +++ b/src/backend/db/sync_state.py @@ -35,6 +35,9 @@ "pack_count", # #1595: packs from `git count-objects -v` "loose_objects", # #1595: loose objects (gc-health signal) "maintenance_failures", # #1595: consecutive failed maintenance attempts + "last_pull_at", # trinity-enterprise#703: the container's pull cycle + "last_pull_status", + "behind_after_pull", "last_check_at", "updated_at", ) @@ -80,6 +83,9 @@ def upsert( pack_count: Optional[int] = None, loose_objects: Optional[int] = None, maintenance_failures: Optional[int] = None, + last_pull_at: Optional[str] = None, + last_pull_status: Optional[str] = None, + behind_after_pull: Optional[int] = None, last_check_at: Optional[str] = None, ) -> Dict: """Upsert a sync-state row. @@ -124,6 +130,11 @@ def _merged(field: str, new_value): # 0 is a meaningful reset here (post-success), not "unset" — it # passes _merged as-is; only a true None falls back to prior. "maintenance_failures": _merged("maintenance_failures", maintenance_failures) or 0, + # trinity-enterprise#703: the pull cycle's own outcome. It never + # touches consecutive_failures — that counter is the PUSH health. + "last_pull_at": _merged("last_pull_at", last_pull_at), + "last_pull_status": _merged("last_pull_status", last_pull_status), + "behind_after_pull": _merged("behind_after_pull", behind_after_pull), "last_check_at": last_check_at or now, "updated_at": now, } diff --git a/src/backend/db/tables.py b/src/backend/db/tables.py index 31fe22bd20..f2ef8c11ac 100644 --- a/src/backend/db/tables.py +++ b/src/backend/db/tables.py @@ -974,6 +974,7 @@ def process_bind_param(self, value, dialect): Column("github_pat_encrypted", Text), Column("auto_sync_enabled", Integer), Column("freeze_schedules_if_sync_failing", Integer), + Column("pull_sync_enabled", Integer), # trinity-enterprise#703 ) agent_sync_state = Table( @@ -1000,6 +1001,9 @@ def process_bind_param(self, value, dialect): Column("pack_count", Integer), # #1595: packs from `git count-objects -v` Column("loose_objects", Integer), # #1595: loose objects (gc-health signal) Column("maintenance_failures", Integer), # #1595: consecutive failed maintenance + Column("last_pull_at", Text), # trinity-enterprise#703: the container's pull cycle + Column("last_pull_status", Text), + Column("behind_after_pull", Integer), Column("last_check_at", Text), Column("updated_at", Text), ) diff --git a/src/backend/db_models.py b/src/backend/db_models.py index f23c241e37..a5154c4578 100644 --- a/src/backend/db_models.py +++ b/src/backend/db_models.py @@ -330,6 +330,7 @@ class AgentGitConfig(BaseModel): # #389 sync health observability auto_sync_enabled: bool = False freeze_schedules_if_sync_failing: bool = False + pull_sync_enabled: bool = False # trinity-enterprise#703: the container's pull cycle class GitSyncResult(BaseModel): diff --git a/src/backend/migrations/versions/0076_pull_sync.py b/src/backend/migrations/versions/0076_pull_sync.py new file mode 100644 index 0000000000..fd42dfa66d --- /dev/null +++ b/src/backend/migrations/versions/0076_pull_sync.py @@ -0,0 +1,43 @@ +"""pull_sync — the container's pull cycle (trinity-enterprise#703). + +PostgreSQL half of the dual-track pair; the SQLite half is +``db/migrations.py::pull_sync``. + +Adds ``agent_git_config.pull_sync_enabled`` (the per-agent switch) and +``agent_sync_state.last_pull_at / last_pull_status / behind_after_pull`` (the +pull cycle's outcome), then turns the pull on only where auto-sync is already +on (operator ruling 2026-09-25). + +Revision ID: 0076_pull_sync +Revises: 0075_auto_sync_enabled_backfill +""" +from alembic import op + + +revision = "0076_pull_sync" +down_revision = "0075_auto_sync_enabled_backfill" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # `IF NOT EXISTS`: a fresh PostgreSQL database is built from db/schema.py's + # DDL, which already declares these columns, and only THEN runs revisions. + op.execute( + "ALTER TABLE agent_git_config ADD COLUMN IF NOT EXISTS " + "pull_sync_enabled INTEGER DEFAULT 0" + ) + op.execute("ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS last_pull_at TEXT") + op.execute("ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS last_pull_status TEXT") + op.execute("ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS behind_after_pull INTEGER") + op.execute( + "UPDATE agent_git_config SET pull_sync_enabled = 1 " + "WHERE COALESCE(auto_sync_enabled, 0) = 1" + ) + + +def downgrade() -> None: + op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS behind_after_pull") + op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS last_pull_status") + op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS last_pull_at") + op.execute("ALTER TABLE agent_git_config DROP COLUMN IF EXISTS pull_sync_enabled") diff --git a/src/backend/models.py b/src/backend/models.py index 0b87dc2701..4bad953179 100644 --- a/src/backend/models.py +++ b/src/backend/models.py @@ -3206,6 +3206,11 @@ class FreezeSchedulesToggle(BaseModel): enabled: bool +class PullSyncToggle(BaseModel): + """trinity-enterprise#703: the container's pull cycle on/off.""" + enabled: bool + + # ============================================================================= # Image Generation Models (routers/image_generation.py) # ============================================================================= diff --git a/src/backend/routers/git.py b/src/backend/routers/git.py index 97d93739bf..c8cf5c766d 100644 --- a/src/backend/routers/git.py +++ b/src/backend/routers/git.py @@ -18,6 +18,7 @@ from models import ( AutoSyncToggle, + PullSyncToggle, BindAgentRepoRequest, BindAgentRepoResponse, FreezeSchedulesToggle, @@ -1185,6 +1186,33 @@ async def set_auto_sync_config( return {"agent_name": agent_name, "auto_sync_enabled": body.enabled} +@router.get("/{agent_name}/git/pull-sync") +async def get_pull_sync_config(agent_name: AuthorizedAgentByName): + """trinity-enterprise#703: whether the agent's container pulls origin on its + own. The agent's pull loop reads this every cycle with its own key.""" + config = db.get_git_config(agent_name) + if not config: + raise HTTPException(status_code=404, detail="Git not configured") + return { + "agent_name": agent_name, + "pull_sync_enabled": bool(getattr(config, "pull_sync_enabled", False)), + } + + +@router.put("/{agent_name}/git/pull-sync") +async def set_pull_sync_config( + agent_name: OwnedAgentByName, + body: PullSyncToggle, +): + """trinity-enterprise#703: turn the container's pull cycle on or off; live + on the agent's next pull cycle, no recreate.""" + config = db.get_git_config(agent_name) + if not config: + raise HTTPException(status_code=404, detail="Git not configured") + db.set_git_pull_sync_enabled(agent_name, body.enabled) + return {"agent_name": agent_name, "pull_sync_enabled": body.enabled} + + @router.get("/{agent_name}/git/freeze-schedules-if-failing") async def get_freeze_schedules_config(agent_name: AuthorizedAgentByName): """Return whether scheduled executions should pause when sync is failing.""" diff --git a/src/backend/services/agent_service/crud.py b/src/backend/services/agent_service/crud.py index 35dd1c4b97..a5d8be0ee3 100644 --- a/src/backend/services/agent_service/crud.py +++ b/src/backend/services/agent_service/crud.py @@ -1880,6 +1880,10 @@ def _apply_github_env( config, github_repo_for_agent, github_pat_for_agent, fork_upstream_repo ): env_vars['GIT_SYNC_AUTO'] = 'true' + # trinity-enterprise#703: the pull cycle's fallback env — the DB flag + # written below at creation is the one the loop reads live. + if not config.ephemeral: + env_vars['GIT_SYNC_PULL'] = 'true' # Source mode (default): Track source branch directly for pull-only sync # Legacy mode: Create a unique working branch for bidirectional sync @@ -2594,6 +2598,15 @@ async def _materialize_agent_files( f"Failed to enable freeze-on-sync-failure for {config.name}: {e}" ) + # trinity-enterprise#703: the container pulls origin on its own — for every + # `github:` agent, source mode included (a pull-only agent is exactly the one + # that needs a pull). Ghosts are excluded: their workspace is throwaway. + if github_repo_for_agent and not config.ephemeral: + try: + db.set_git_pull_sync_enabled(config.name, True) + except Exception as e: + logger.error(f"Failed to enable the pull cycle for {config.name}: {e}") + # #2069: the fleet-wide `.gitignore` merge never ran at creation, so the # 15-min in-container auto-sync loop (on from birth for the GIT_SYNC_AUTO # set — non-source/fork `github:` agents, ephemeral ghosts INCLUDED) staged diff --git a/src/backend/services/agent_service/lifecycle.py b/src/backend/services/agent_service/lifecycle.py index 15dcca427c..2bb0f69639 100644 --- a/src/backend/services/agent_service/lifecycle.py +++ b/src/backend/services/agent_service/lifecycle.py @@ -630,6 +630,7 @@ async def restart_agent_internal(agent_name: str, *, stop_timeout: int = 30) -> "GIT_SOURCE_MODE", "GIT_SOURCE_BRANCH", "GIT_SYNC_AUTO", + "GIT_SYNC_PULL", # trinity-enterprise#703 "TRINITY_GIT_BASE_URL", ) @@ -811,6 +812,14 @@ def _gc(key: str): else: env_vars.pop("GIT_SYNC_AUTO", None) + # trinity-enterprise#703: the pull cycle's fallback env, from the DB flag + # alone — same one-writer rule as GIT_SYNC_AUTO; the loop reads the flag + # live every cycle and falls back to this only when the platform is down. + if _gc("pull_sync_enabled"): + env_vars["GIT_SYNC_PULL"] = "true" + else: + env_vars.pop("GIT_SYNC_PULL", None) + # --- optional self-hosted git base URL: refresh from the CURRENT backend # env (the AGENT_TOOL_STALL_LIMIT_S idiom), so pointing the platform at or # away from a gitea/GHES harness takes effect on recreate. diff --git a/src/backend/services/sync_health_service.py b/src/backend/services/sync_health_service.py index 7cf091b5c8..c4df81e488 100644 --- a/src/backend/services/sync_health_service.py +++ b/src/backend/services/sync_health_service.py @@ -241,6 +241,18 @@ def set_websocket_manager(manager): _websocket_manager = manager +# trinity-enterprise#703: the pull cycle's status vocabulary (agent-written). +_PULL_STATUSES = frozenset({"success", "failed", "skipped", "never"}) + + +def _pull_status(value) -> Optional[str]: + return value if isinstance(value, str) and value in _PULL_STATUSES else None + + +def _pull_text(value, limit: int) -> Optional[str]: + return value[:limit] if isinstance(value, str) and value else None + + class SyncHealthService: """Background service that keeps agent_sync_state fresh and raises alerts.""" @@ -454,6 +466,12 @@ async def _sync_agent(self, config) -> None: pack_count=pack_count, # #1595 loose_objects=loose_objects, # #1595 maintenance_failures=maintenance_failures, # #1595 + # trinity-enterprise#703: the container's pull cycle. Agent-written + # JSON, so the status is bounded to the vocabulary and the counter + # coerced like its neighbours. + last_pull_at=_pull_text(sync_state.get("last_pull_at"), 64), + last_pull_status=_pull_status(sync_state.get("last_pull_status")), + behind_after_pull=_coerce_nonneg_int(sync_state.get("behind_after_pull")), last_check_at=utc_now_iso(), ) diff --git a/src/frontend/src/components/GitSyncSettingsPanel.vue b/src/frontend/src/components/GitSyncSettingsPanel.vue index 5549dee70f..040d4aa7f2 100644 --- a/src/frontend/src/components/GitSyncSettingsPanel.vue +++ b/src/frontend/src/components/GitSyncSettingsPanel.vue @@ -62,6 +62,27 @@ /> +
+ +

+ Brings in edits people and other agents push to the repository. Never + runs while the agent is working, and never discards its own changes. +

+ +
+
({ - useAgentsStore: () => ({ getGitAutoSync, setGitAutoSync, getGitFreezeSchedules, setGitFreezeSchedules }), + useAgentsStore: () => ({ + getGitAutoSync, setGitAutoSync, getGitFreezeSchedules, setGitFreezeSchedules, + getGitPullSync, setGitPullSync, + }), })) // eslint-disable-next-line import/first @@ -38,6 +43,7 @@ beforeEach(() => { vi.clearAllMocks() getGitAutoSync.mockResolvedValue({ agent_name: 'a1', auto_sync_enabled: true }) getGitFreezeSchedules.mockResolvedValue({ agent_name: 'a1', freeze_schedules_if_sync_failing: false }) + getGitPullSync.mockResolvedValue({ agent_name: 'a1', pull_sync_enabled: true }) }) describe('GitSyncSettingsPanel', () => { @@ -59,6 +65,26 @@ describe('GitSyncSettingsPanel', () => { expect(notify).toHaveBeenCalledWith(expect.stringMatching(/next cycle is skipped/), 'success') }) + it('the pull toggle opens on the persisted value and turning it off writes false', async () => { + // trinity-enterprise#703 + setGitPullSync.mockResolvedValue({ agent_name: 'a1', pull_sync_enabled: false }) + const wrapper = await mountPanel() + expect(toggle(wrapper, 'pull-sync-toggle').attributes('aria-checked')).toBe('true') + await toggle(wrapper, 'pull-sync-toggle').trigger('click') + await flushPromises() + expect(setGitPullSync).toHaveBeenCalledWith('a1', false) + expect(toggle(wrapper, 'pull-sync-toggle').attributes('aria-checked')).toBe('false') + }) + + it('a failed pull save keeps the prior value and says so beside the control', async () => { + setGitPullSync.mockRejectedValue(httpError(403, 'Only the owner can change this')) + const wrapper = await mountPanel() + await toggle(wrapper, 'pull-sync-toggle').trigger('click') + await flushPromises() + expect(toggle(wrapper, 'pull-sync-toggle').attributes('aria-checked')).toBe('true') + expect(wrapper.text()).toContain("Couldn't turn pulling off: Only the owner can change this") + }) + it('turning the schedule pause on writes true', async () => { setGitFreezeSchedules.mockResolvedValue({ agent_name: 'a1', freeze_schedules_if_sync_failing: true }) const wrapper = await mountPanel() @@ -80,6 +106,7 @@ describe('GitSyncSettingsPanel', () => { it('an agent with no GitHub binding gets the not-connected copy, not an error', async () => { getGitAutoSync.mockRejectedValue(httpError(404, 'Git not configured')) getGitFreezeSchedules.mockRejectedValue(httpError(404, 'Git not configured')) + getGitPullSync.mockRejectedValue(httpError(404, 'Git not configured')) const wrapper = await mountPanel() expect(wrapper.find('[data-testid="git-sync-unbound"]').exists()).toBe(true) expect(wrapper.find('[data-testid="load-failed"]').exists()).toBe(false) diff --git a/tests/registry.json b/tests/registry.json index e87ff280fe..77913b4022 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -3484,6 +3484,17 @@ "git" ], "description": "Git-sync write access is validated where GitHub enforces it (#2107): probe_push_access runs REAL `git push --dry-run` against a local fake smart-HTTP server — 403 + `Write access to repository not granted.` → denied with GitHub's line verbatim; a valid receive-pack advertisement → ok with nothing POSTed; 5xx / unreachable → transient; the PAT reaches the server as an Authorization header and is never on git's argv; is_push_denied over refusal vs non-refusal texts. Create-path cases (400 before any container, transient non-blocking, source mode not probed) in test_1484_create_agent_characterization.py; the named sync_failing item in test_sync_health_service.py. 20 of 23 red with the fix reverted." + }, + { + "file": "unit/test_ent703_pull_cycle.py", + "feature": "trinity-enterprise#703", + "added": "2026-09-25", + "categories": [ + "agent-server", + "git", + "migrations" + ], + "description": "The container's pull cycle (ent#703, invariant G3) on real repos with a human clone pushing underneath: behind+clean fast-forwards; a local commit is rebased on top; uncommitted edits survive a pull (stashed and re-applied); incoming changes colliding with uncommitted edits are UNDONE — same HEAD, same edits, empty stash, failure recorded (the autostash trap; red against a naive --autostash mutant); a rebase conflict is aborted and recorded; never while an execution runs, including one starting mid-cycle, and an unknown registry counts as busy; a busy repo is skipped without a state write; a pull never touches the push's fields. The loop gate reads the flag live on /git/pull-sync with the env fallback; the interval defaults to the push interval. Both migration tracks: columns + backfill only where auto-sync is on." } ] } diff --git a/tests/unit/test_1484_create_agent_characterization.py b/tests/unit/test_1484_create_agent_characterization.py index 3d5d4b94bc..6ce6c0677b 100644 --- a/tests/unit/test_1484_create_agent_characterization.py +++ b/tests/unit/test_1484_create_agent_characterization.py @@ -1445,3 +1445,18 @@ async def test_ent705_a_fork_reports_the_kind_that_was_asked_for(crud_env, kind, assert decision["kind"] == expected assert decision["reason"] == "fork-to-own: the agent owns its fork" ctx["git_service"].probe_push_access.assert_not_awaited() + + +# =========================================================================== +# trinity-enterprise#703 — the container pulls origin on its own +# =========================================================================== + +@pytest.mark.asyncio +@pytest.mark.parametrize("kw", [{}, {"kind": "deployment"}], ids=["agent", "deployment"]) +async def test_ent703_a_github_create_turns_the_pull_cycle_on(crud_env, monkeypatch, kw): + """Source mode included: a pull-only agent is exactly the one that needs a pull.""" + crud, ctx = crud_env + await _create_default(crud, ctx, monkeypatch, "gc-pull", **kw) + + assert _agent_run_kwargs(ctx)["environment"]["GIT_SYNC_PULL"] == "true" + ctx["db"].set_git_pull_sync_enabled.assert_called_once_with("gc-pull", True) diff --git a/tests/unit/test_ent109_git_env_seam.py b/tests/unit/test_ent109_git_env_seam.py index 50554d1593..1684667fa9 100644 --- a/tests/unit/test_ent109_git_env_seam.py +++ b/tests/unit/test_ent109_git_env_seam.py @@ -366,6 +366,19 @@ def test_db_flag_alone_enables_auto_push(self, lc_env): assert env["GIT_SYNC_AUTO"] == "true" db.set_git_auto_sync_enabled.assert_not_called() + @pytest.mark.parametrize("flag, expected", [(True, "true"), (False, None)]) + def test_the_pull_env_follows_the_db_flag_alone(self, lc_env, flag, expected): + """trinity-enterprise#703: GIT_SYNC_PULL is derived like GIT_SYNC_AUTO — + the DB flag alone, never written back.""" + lc, db, _rg = lc_env + db.get_git_config.return_value = _row(source_mode=True, pull_sync_enabled=flag) + + env = _carried(GIT_SYNC_PULL="true") + lc._apply_git_env_from_db("a1", env, pat_gate="per_agent_only") + + assert env.get("GIT_SYNC_PULL") == expected + db.set_git_pull_sync_enabled.assert_not_called() + def test_neither_source_clears_a_stale_flag(self, lc_env): lc, db, _rg = lc_env db.get_git_config.return_value = _row(auto_sync_enabled=False) @@ -550,7 +563,8 @@ def test_owned_key_set_is_complete(self, lc_env, monkeypatch): """`_GIT_ENV_KEYS` drives the clear sweep; a var the helper writes but forgets to list would never be cleared.""" lc, db, rg = lc_env - db.get_git_config.return_value = _row(auto_sync_enabled=True) + # every owned var written: auto-sync and (trinity-enterprise#703) pull on + db.get_git_config.return_value = _row(auto_sync_enabled=True, pull_sync_enabled=True) db.get_agent_github_pat.return_value = "ghp_PER_AGENT" rg.get_github_pat_for_agent.return_value = "ghp_PER_AGENT" monkeypatch.setenv("TRINITY_GIT_BASE_URL", "http://gitea:3000") diff --git a/tests/unit/test_ent703_pull_cycle.py b/tests/unit/test_ent703_pull_cycle.py new file mode 100644 index 0000000000..0e61015ffd --- /dev/null +++ b/tests/unit/test_ent703_pull_cycle.py @@ -0,0 +1,316 @@ +""" +trinity-enterprise#703 — the container's pull cycle (invariant G3: human and +fleet work reaches the agent within a bound). + +Real repos: a bare "origin", the agent's clone, and a "human" clone that pushes +underneath the agent. The cycle is `routers/git.py::_run_pull_once`: + - behind with nothing local → fast-forward + - behind with a local commit → rebase, the local commit on top + - behind with uncommitted edits → stashed and re-applied, edits intact + - incoming changes colliding with uncommitted edits → the pull is UNDONE: + same HEAD, same edits, failure recorded (never lost to the stash) + - a rebase conflict → aborted, recorded, nothing reset + - an execution in flight (or unknown) → skipped, the tree untouched + - the push's own fields are never touched by a pull +Plus the loop gate (flag read live, env fallback) and both migration tracks. +""" +from __future__ import annotations + +import asyncio +import importlib.util +import sqlite3 +import subprocess +from pathlib import Path + +import httpx +import pytest + +# `agent_server` is the real base-image package, installed by +# tests/unit/conftest.py::_preload_real_agent_server. +from agent_server import auto_sync +from agent_server.routers import git as git_router + +_ROOT = Path(__file__).resolve().parents[2] + + +def _git(repo, *args, check=True): + return subprocess.run(["git", *args], cwd=str(repo), capture_output=True, + text=True, timeout=20, check=check) + + +def _out(repo, *args): + return _git(repo, *args).stdout.strip() + + +def _identity(repo): + _git(repo, "config", "user.email", "t@example.com") + _git(repo, "config", "user.name", "T") + _git(repo, "config", "commit.gpgsign", "false") + + +@pytest.fixture +def world(tmp_path, monkeypatch): + """(agent, origin, human); nothing running on the agent by default.""" + origin, agent, human = tmp_path / "origin.git", tmp_path / "agent", tmp_path / "human" + origin.mkdir() + agent.mkdir() + _git(origin, "init", "-q", "--bare", "-b", "main") + _git(agent, "init", "-q", "-b", "main") + _identity(agent) + _git(agent, "remote", "add", "origin", str(origin)) + (agent / "role.md").write_text("role v1\n") + (agent / "notes.md").write_text("notes v1\n") + (agent / ".gitignore").write_text(".trinity/\n") + _git(agent, "add", "-A") + _git(agent, "commit", "-q", "-m", "init") + _git(agent, "push", "-q", "-u", "origin", "main") + (agent / ".trinity").mkdir() + _git(tmp_path, "clone", "-q", str(origin), str(human)) + _identity(human) + monkeypatch.setattr(git_router, "_executions_in_flight", lambda: 0) + return agent, origin, human + + +def _human_push(human, name, content): + _git(human, "pull", "-q", "--rebase", "origin", "main") + (human / name).write_text(content) + _git(human, "add", name) + _git(human, "commit", "-q", "-m", f"human: {name}") + _git(human, "push", "-q", "origin", "HEAD:main") + return _out(human, "rev-parse", "HEAD") + + +def _state(agent): + return git_router._read_sync_state_file(agent) + + +class TestPull: + def test_behind_and_clean_fast_forwards(self, world): + agent, _, human = world + sha = _human_push(human, "role.md", "role v2 — edited on GitHub\n") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "success" + assert _out(agent, "rev-parse", "HEAD") == sha + assert (agent / "role.md").read_text() == "role v2 — edited on GitHub\n" + st = _state(agent) + assert st["last_pull_status"] == "success" and st["behind_after_pull"] == 0 + assert st["last_pull_at"] + + def test_a_local_commit_is_rebased_on_top(self, world): + agent, _, human = world + sha = _human_push(human, "role.md", "role v2\n") + (agent / "notes.md").write_text("notes v2 — the agent's\n") + _git(agent, "commit", "-qam", "agent: notes") + + assert git_router._run_pull_once(agent)["status"] == "success" + assert _out(agent, "rev-parse", "HEAD~1") == sha + assert _out(agent, "log", "-1", "--format=%s") == "agent: notes" + assert (agent / "role.md").read_text() == "role v2\n" + + def test_uncommitted_edits_survive_a_pull(self, world): + """A source-mode agent never commits: its standing edits must ride through.""" + agent, _, human = world + sha = _human_push(human, "role.md", "role v2\n") + (agent / "notes.md").write_text("notes — uncommitted\n") + + assert git_router._run_pull_once(agent)["status"] == "success" + assert _out(agent, "rev-parse", "HEAD") == sha + assert (agent / "notes.md").read_text() == "notes — uncommitted\n" + assert (agent / "role.md").read_text() == "role v2\n" + assert _out(agent, "stash", "list") == "" + + def test_incoming_changes_colliding_with_edits_are_undone_not_lost(self, world): + """The autostash trap: its re-apply conflicts, leaves the edits only in + the stash, and reports success. Here the pull is undone instead.""" + agent, _, human = world + before = _out(agent, "rev-parse", "HEAD") + _human_push(human, "notes.md", "notes — human rewrite\n") + (agent / "notes.md").write_text("notes — agent's uncommitted edit\n") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert result["error"] == "local edits conflict with incoming changes on main" + assert _out(agent, "rev-parse", "HEAD") == before + assert (agent / "notes.md").read_text() == "notes — agent's uncommitted edit\n" + assert _out(agent, "stash", "list") == "" + assert _state(agent)["behind_after_pull"] == 1 + + def test_a_rebase_conflict_is_aborted_and_recorded(self, world): + agent, _, human = world + _human_push(human, "notes.md", "notes — human\n") + (agent / "notes.md").write_text("notes — agent\n") + _git(agent, "commit", "-qam", "agent: notes") + local = _out(agent, "rev-parse", "HEAD") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert result["error"] == "diverged: rebase conflict on main" + assert _out(agent, "rev-parse", "HEAD") == local + assert not (agent / ".git" / "rebase-merge").exists() + assert (agent / "notes.md").read_text() == "notes — agent\n" + + @pytest.mark.parametrize("running, reason", [(2, "execution in flight"), + (None, "execution state unknown")]) + def test_never_while_an_execution_runs(self, world, monkeypatch, running, reason): + agent, _, human = world + before = _out(agent, "rev-parse", "HEAD") + _human_push(human, "role.md", "role v2\n") + monkeypatch.setattr(git_router, "_executions_in_flight", lambda: running) + + result = git_router._run_pull_once(agent) + + assert result == {"status": "skipped", "error": reason} + assert _out(agent, "rev-parse", "HEAD") == before + + def test_an_execution_starting_mid_cycle_stops_it_before_the_tree(self, world, monkeypatch): + agent, _, human = world + before = _out(agent, "rev-parse", "HEAD") + _human_push(human, "role.md", "role v2\n") + answers = iter([0, 1]) # idle at the start, busy after the fetch + monkeypatch.setattr(git_router, "_executions_in_flight", lambda: next(answers)) + + result = git_router._run_pull_once(agent) + + assert result["status"] == "skipped" + assert _out(agent, "rev-parse", "HEAD") == before + + def test_a_busy_repo_is_skipped_without_writing_state(self, world): + agent, _, _ = world + assert git_router._REPO_LOCK.acquire(blocking=False) + try: + assert git_router._run_pull_once(agent) == {"status": "skipped", "reason": "repo_busy"} + finally: + git_router._REPO_LOCK.release() + assert _state(agent)["last_pull_status"] == "never" + + def test_a_pull_never_touches_the_push_fields(self, world): + agent, _, human = world + git_router._write_sync_state_file(agent, "failed", last_error_summary="push boom") + _human_push(human, "role.md", "role v2\n") + + git_router._run_pull_once(agent) + + st = _state(agent) + assert (st["last_sync_status"], st["consecutive_failures"], st["last_error_summary"]) == ( + "failed", 1, "push boom") + assert st["last_pull_status"] == "success" + + def test_up_to_date_and_unpushed_branch_are_quiet_successes(self, world): + agent, _, _ = world + assert git_router._run_pull_once(agent)["status"] == "success" + _git(agent, "checkout", "-q", "-b", "trinity/agent/new") + assert git_router._run_pull_once(agent)["status"] == "success" + assert _state(agent)["behind_after_pull"] == 0 + + +class TestLoopGate: + @pytest.fixture(autouse=True) + def _env(self, monkeypatch): + monkeypatch.setenv("TRINITY_BACKEND_URL", "http://backend:8000") + monkeypatch.setenv("TRINITY_MCP_API_KEY", "trinity_mcp_test") + monkeypatch.setenv("AGENT_NAME", "a1") + monkeypatch.delenv("GIT_SYNC_PULL", raising=False) + monkeypatch.setattr(auto_sync, "_last_pull_resolved", None) + + def _cycle(self, repo, answer): + seen, ran = [], [] + + def handler(request): + seen.append(str(request.url)) + if isinstance(answer, Exception): + raise answer + return httpx.Response(answer[0], json=answer[1]) + + async def go(): + async with httpx.AsyncClient(transport=httpx.MockTransport(handler)) as c: + return await auto_sync.run_one_pull_cycle(c, repo, lambda h: ran.append(h) or {"status": "success"}) + + result = asyncio.new_event_loop().run_until_complete(go()) + return result, ran, seen + + def test_the_flag_is_read_live_on_its_own_endpoint(self, tmp_path): + (tmp_path / ".git").mkdir() + _, ran, seen = self._cycle(tmp_path, (200, {"pull_sync_enabled": True})) + assert ran == [tmp_path] + assert seen == ["http://backend:8000/api/agents/a1/git/pull-sync"] + + def test_off_skips(self, tmp_path): + (tmp_path / ".git").mkdir() + _, ran, _ = self._cycle(tmp_path, (200, {"pull_sync_enabled": False})) + assert ran == [] + + @pytest.mark.parametrize("env, expected", [("true", True), (None, False)]) + def test_platform_down_falls_back_to_the_env(self, tmp_path, monkeypatch, env, expected): + (tmp_path / ".git").mkdir() + if env: + monkeypatch.setenv("GIT_SYNC_PULL", env) + _, ran, _ = self._cycle(tmp_path, httpx.ConnectError("down")) + assert bool(ran) is expected + + def test_pull_interval_defaults_to_the_push_interval(self, monkeypatch): + monkeypatch.setenv("GIT_SYNC_INTERVAL_SECONDS", "600") + monkeypatch.delenv("GIT_SYNC_PULL_INTERVAL_SECONDS", raising=False) + assert auto_sync.get_pull_interval_seconds() == 600 + monkeypatch.setenv("GIT_SYNC_PULL_INTERVAL_SECONDS", "300") + assert auto_sync.get_pull_interval_seconds() == 300 + + +# --------------------------------------------------------------------------- +# Both migration tracks: columns + the backfill (pull on where auto-sync is on) +# --------------------------------------------------------------------------- + +@pytest.fixture() +def pre_migration_db(): + """The two tables as they stood BEFORE this migration.""" + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE agent_git_config (agent_name TEXT PRIMARY KEY, " + "auto_sync_enabled INTEGER DEFAULT 0)") + conn.execute("CREATE TABLE agent_sync_state (agent_name TEXT PRIMARY KEY)") + conn.executemany("INSERT INTO agent_git_config VALUES (?, ?)", + [("pushing", 1), ("quiet", 0), ("unset", None)]) + conn.commit() + yield conn + conn.close() + + +def _pull_flags(conn): + return dict(conn.execute("SELECT agent_name, pull_sync_enabled FROM agent_git_config")) + + +def test_sqlite_migration_adds_columns_and_backfills_only_auto_sync_agents(pre_migration_db): + from db.migrations import _migrate_pull_sync + _migrate_pull_sync(pre_migration_db.cursor(), pre_migration_db) + _migrate_pull_sync(pre_migration_db.cursor(), pre_migration_db) # idempotent + assert _pull_flags(pre_migration_db) == {"pushing": 1, "quiet": 0, "unset": 0} + cols = {r[1] for r in pre_migration_db.execute("PRAGMA table_info(agent_sync_state)")} + assert {"last_pull_at", "last_pull_status", "behind_after_pull"} <= cols + + +def test_alembic_revision_chains_on_the_head_and_backfills_the_same(pre_migration_db, monkeypatch): + path = _ROOT / "src/backend/migrations/versions/0076_pull_sync.py" + spec = importlib.util.spec_from_file_location("rev0076_703", path) + rev = importlib.util.module_from_spec(spec) + spec.loader.exec_module(rev) + assert (rev.revision, rev.down_revision) == ("0076_pull_sync", "0075_auto_sync_enabled_backfill") + # PostgreSQL's `ADD COLUMN IF NOT EXISTS` is not SQLite; run the statements + # through a translating executor so the backfill is exercised for real. + executed = [] + + def fake_execute(sql): + executed.append(sql) + stmt = sql.replace(" IF NOT EXISTS", "") + pre_migration_db.execute(stmt) + + monkeypatch.setattr(rev.op, "execute", fake_execute) + rev.upgrade() + assert _pull_flags(pre_migration_db) == {"pushing": 1, "quiet": 0, "unset": 0} + assert all("IF NOT EXISTS" in s for s in executed if "ADD COLUMN" in s) + + +def test_the_migration_is_registered_on_the_sqlite_track(): + from db.migrations import MIGRATIONS + assert "pull_sync" in [name for name, _ in MIGRATIONS] diff --git a/tests/unit/test_sync_health_service.py b/tests/unit/test_sync_health_service.py index 0551de1ed9..0924f73078 100644 --- a/tests/unit/test_sync_health_service.py +++ b/tests/unit/test_sync_health_service.py @@ -169,6 +169,43 @@ async def test_unreachable_agent_is_skipped(self, service, seed_agent): assert db.get_sync_state("alpha") is None +class TestPullOutcomePersisted: + """trinity-enterprise#703: the pull cycle's outcome reaches the row, bounded.""" + + @pytest.mark.asyncio + async def test_pull_fields_are_persisted_and_the_push_counter_untouched(self, service, seed_agent): + seed_agent("alpha") + payload = _status_payload(status="success") + payload["sync_state"].update({ + "last_pull_at": "2026-09-25T12:00:00+00:00", + "last_pull_status": "failed", + "behind_after_pull": 3, + }) + with patch.object(service, "_fetch_git_status", AsyncMock(return_value=payload)): + await service._poll_cycle() + from database import db + row = db.get_sync_state("alpha") + assert (row["last_pull_at"], row["last_pull_status"], row["behind_after_pull"]) == ( + "2026-09-25T12:00:00+00:00", "failed", 3) + assert row["consecutive_failures"] == 0 # a failed PULL is not a failed push + + @pytest.mark.asyncio + async def test_agent_written_garbage_is_dropped(self, service, seed_agent): + seed_agent("alpha") + payload = _status_payload(status="success") + payload["sync_state"].update({ + "last_pull_status": "pwned; DROP TABLE", + "behind_after_pull": "lots", + "last_pull_at": {"not": "a string"}, + }) + with patch.object(service, "_fetch_git_status", AsyncMock(return_value=payload)): + await service._poll_cycle() + from database import db + row = db.get_sync_state("alpha") + assert (row["last_pull_at"], row["last_pull_status"], row["behind_after_pull"]) == ( + None, None, None) + + class TestOperatorQueueEmission: """sync_failing entry emitted when consecutive_failures crosses 3.""" From fa8f2513f1d8c24b62b756af8fd445d88dd35ff6 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Fri, 25 Sep 2026 17:35:42 +0300 Subject: [PATCH 2/8] fix(schema): keep the FK strip working on agent_git_config for PostgreSQL (trinity-enterprise#703) The inline comment on `pull_sync_enabled` sat between the comma and the FOREIGN KEY clause. `_PG_TABLE_SUBS` strips that clause only when it directly follows the comma, so the clause survived without its REFERENCES and PostgreSQL rejected the table ("syntax error at or near ')'"), failing 18 requires_postgres tests in schema-parity. The comment moves above the column. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/backend/db/schema.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/backend/db/schema.py b/src/backend/db/schema.py index 8c74ca0deb..66a825a219 100644 --- a/src/backend/db/schema.py +++ b/src/backend/db/schema.py @@ -1146,7 +1146,10 @@ github_pat_encrypted TEXT, auto_sync_enabled INTEGER DEFAULT 0, freeze_schedules_if_sync_failing INTEGER DEFAULT 0, - pull_sync_enabled INTEGER DEFAULT 0, -- trinity-enterprise#703 + -- trinity-enterprise#703. No trailing comment on the column above the + -- FK clause: _PG_TABLE_SUBS strips the clause only when it follows + -- the comma directly, so a comment there breaks PostgreSQL. + pull_sync_enabled INTEGER DEFAULT 0, FOREIGN KEY (agent_name) REFERENCES agent_ownership(agent_name) ) """, From edf8edd225f3b06ab9d4fe3a19adf5c796c62c19 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Mon, 28 Sep 2026 11:03:16 +0300 Subject: [PATCH 3/8] fix(git-sync): pull cycle never strands local work, counts queued turns, brings main to working branches (trinity-enterprise#703) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #3021 review: - _with_stash wraps every post-stash step: a git child that times out (run_registered raises TimeoutExpired) goes back to the pre-pull HEAD and re-applies the stash; when that is impossible the error says the edits are kept in `git stash`. - reset --hard's return code is checked. A failed undo stops with an error naming it instead of leaving UU files behind silently. - Both cycles refuse to run over unmerged paths. The push cycle checks before `git add -A`, which would stage conflict markers and push them to origin as a successful sync. - The execution gate counts register_pending entries (#2433), so a turn queued on the chat lock or the headless pool is busy, not idle. - The pull reaps stale lock litter first, as the push cycle does; a pull-only agent has no push cycle to do it. - sync-state gains last_pull_error streak fields: consecutive pull failures / skips and last_successful_pull_at. Ruling on the intent question: a trinity/* working branch only ever pulled itself, so human pushes to main never arrived (G3). The cycle now also merges origin/main (via _get_pull_branch) into the working branch — a merge, not a rebase, since the branch is already pushed; a conflict is aborted and recorded. Nine real-repo tests, all red before. Co-Authored-By: Claude Opus 5.5 --- docker/base-image/agent_server/routers/git.py | 257 +++++++++++++----- tests/unit/test_ent703_pull_cycle.py | 185 +++++++++++++ 2 files changed, 381 insertions(+), 61 deletions(-) diff --git a/docker/base-image/agent_server/routers/git.py b/docker/base-image/agent_server/routers/git.py index 29f4a0298c..891a384a84 100644 --- a/docker/base-image/agent_server/routers/git.py +++ b/docker/base-image/agent_server/routers/git.py @@ -15,7 +15,7 @@ import uuid from datetime import datetime, timedelta, timezone from pathlib import Path -from typing import Callable, Dict, Optional +from typing import Callable, Dict, List, Optional from fastapi import APIRouter, HTTPException from fastapi.responses import JSONResponse @@ -828,35 +828,70 @@ def _rebase_onto_remote(home_dir: Path, branch: str) -> Optional[str]: def _executions_in_flight() -> Optional[int]: - """How many executions are running on this agent, or None if unknown. - Indirection so the pull cycle's execution gate is unit-testable.""" + """How many executions are running OR accepted-but-not-yet-spawned on this + agent, or None if unknown. Indirection so the pull cycle's execution gate is + unit-testable. + + Pending entries count (#2433): a turn waiting on the chat lock or the + headless pool is not in `list_running()` yet, and a rebase that lands in + that window changes the files the turn is about to read. + """ try: from ..services.process_registry import get_process_registry - return len(get_process_registry().list_running()) + registry = get_process_registry() + return len(registry.list_running()) + len(registry.list_pending_ids()) except Exception: # noqa: BLE001 — unknown is treated as busy by the caller logger.debug("pull: could not read the process registry", exc_info=True) return None +def _unmerged_paths(home_dir: Path) -> List[str]: + """Paths the index holds as unmerged (conflict stages). Committing them — + `git add -A` resolves a `UU` file by staging its conflict markers — would + push the markers to origin, so both cycles refuse to run while any exist.""" + out = run_registered( + ["git", "diff", "--name-only", "--diff-filter=U"], + cwd=str(home_dir), timeout=10, + ) + return [line for line in (out.stdout or "").splitlines() if line.strip()] + + def _record_pull(home_dir: Path, status: str, *, error: Optional[str] = None, behind: Optional[int] = None) -> Dict: """Persist one pull outcome WITHOUT touching the push fields — the push's - `consecutive_failures` / `last_sync_*` stay the push's (trinity-enterprise#703).""" - _patch_sync_state(home_dir, { - "last_pull_at": datetime.now(timezone.utc).isoformat(), + `consecutive_failures` / `last_sync_*` stay the push's (trinity-enterprise#703). + + Carries its own streaks (`consecutive_pull_failures` / `_skips`) and the + last success time, so a pull that has not landed for hours is visible as + such rather than as one `failed` row that looks like the first.""" + prior = _read_sync_state_file(home_dir) + now = datetime.now(timezone.utc).isoformat() + + def _count(key: str) -> int: + value = prior.get(key) + return value if isinstance(value, int) and value >= 0 else 0 + + updates = { + "last_pull_at": now, "last_pull_status": status, "last_pull_error": error, "behind_after_pull": behind, - }) + "consecutive_pull_failures": _count("consecutive_pull_failures") + 1 if status == "failed" else 0, + "consecutive_pull_skips": _count("consecutive_pull_skips") + 1 if status == "skipped" else 0, + } + if status == "success": + updates["last_successful_pull_at"] = now + _patch_sync_state(home_dir, updates) result = {"status": status} if error: result["error"] = error return result -def _integrate_remote(home_dir: Path, branch: str, ahead: int) -> Optional[str]: - """Bring `origin/` into the checked-out branch without ever losing - local work (trinity-enterprise#703). Returns None, or the error recorded. +def _with_stash(home_dir: Path, branch: str, step) -> Optional[str]: + """Run `step()` — something that moves HEAD, or fails and leaves it where it + was — with uncommitted tracked edits set aside and put back afterwards. + Returns None, or the error recorded (trinity-enterprise#703). Uncommitted tracked edits are stashed EXPLICITLY — not `--autostash`: when incoming commits touch the same files, autostash's re-apply conflicts and @@ -864,11 +899,18 @@ def _integrate_remote(home_dir: Path, branch: str, ahead: int) -> Optional[str]: files silently lose them. Here a conflicting re-apply is undone: back to the pre-pull HEAD, where the stash applies cleanly to its own base, and the failure is recorded. Untracked files are never stashed; one an incoming - commit would overwrite fails the fast-forward/rebase instead. + commit would overwrite fails the step instead. + + Every exit path — including a git child that times out (`run_registered` + raises `TimeoutExpired`) — either puts the edits back or says in the + returned error that they are still in `git stash`. A failed `reset --hard` + stops here and says so: the tree may still hold conflict markers, which the + push cycle refuses to commit (`_unmerged_paths`). """ def git(*args, timeout=60): return run_registered(["git", *args], cwd=str(home_dir), timeout=timeout) + kept = "; local edits are kept in `git stash`" pre_head = git("rev-parse", "HEAD", timeout=10).stdout.strip() dirty = git("status", "--porcelain", "--untracked-files=no", timeout=10).stdout.strip() stashed = False @@ -878,55 +920,111 @@ def git(*args, timeout=60): return _summarize_git_error(push.stderr or push.stdout or "stash failed") stashed = True - if ahead == 0: - step = git("merge", "--ff-only", f"origin/{branch}") - err = None if step.returncode == 0 else _summarize_git_error( - step.stderr or step.stdout or "fast-forward failed") - else: - err = _rebase_onto_remote(home_dir, branch) # aborts itself on failure - - if err: - if stashed: # nothing moved (ff refused / rebase aborted): put the edits back - git("stash", "pop", timeout=30) - return err - - if stashed: - pop = git("stash", "pop", timeout=30) - if pop.returncode != 0: - # The edits collide with what came in. Undo the pull — the stash is - # still there — and re-apply on the base it was taken from. - git("reset", "--hard", pre_head, timeout=30) - back = git("stash", "pop", timeout=30) - if back.returncode != 0: - return ("local edits conflict with incoming changes on " - f"{branch}; they are kept in `git stash`") + try: + err = step() + if err: + # Nothing moved (the step aborted itself): put the edits back. + if stashed and git("stash", "pop", timeout=30).returncode == 0: + stashed = False + return err + (kept if stashed else "") + if not stashed: + return None + if git("stash", "pop", timeout=30).returncode == 0: + stashed = False + return None + # The edits collide with what came in. Undo the pull — the stash is + # still there — and re-apply on the base it was taken from. + reset = git("reset", "--hard", pre_head, timeout=30) + if reset.returncode != 0: + return (f"local edits conflict with incoming changes on {branch} and the " + f"pull could not be undone ({_summarize_git_error(reset.stderr or reset.stdout or 'reset failed')})" + + kept) + if git("stash", "pop", timeout=30).returncode == 0: + stashed = False return f"local edits conflict with incoming changes on {branch}" - return None + return f"local edits conflict with incoming changes on {branch}" + kept + except subprocess.TimeoutExpired as exc: + verb = (exc.cmd or ["git", "command"])[1:2] or ["command"] + err = f"git {verb[0]} timed out" + if stashed: + # Best effort back to the base the stash was taken on, then re-apply. + try: + if git("reset", "--hard", pre_head, timeout=30).returncode == 0 \ + and git("stash", "pop", timeout=30).returncode == 0: + stashed = False + except subprocess.TimeoutExpired: + pass + return err + (kept if stashed else "") + + +def _integrate_remote(home_dir: Path, branch: str, ahead: int) -> Optional[str]: + """Bring `origin/` into the checked-out branch without ever losing + local work: a fast-forward when nothing is committed locally, a rebase + (aborted on conflict) when something is. See `_with_stash`.""" + def step() -> Optional[str]: + if ahead == 0: + ff = run_registered(["git", "merge", "--ff-only", f"origin/{branch}"], + cwd=str(home_dir), timeout=60) + return None if ff.returncode == 0 else _summarize_git_error( + ff.stderr or ff.stdout or "fast-forward failed") + return _rebase_onto_remote(home_dir, branch) # aborts itself on failure + + return _with_stash(home_dir, branch, step) + + +def _integrate_source(home_dir: Path, branch: str, source: str) -> Optional[str]: + """Merge `origin/` into a working branch (PR #3021 ruling). + + A `trinity/*` working branch is the agent's own, so pulling it alone never + delivers what humans push to `main` (invariant G3). A MERGE, not a rebase: + the working branch is already pushed, and rewriting it would make the next + push cycle rebase it back. A conflict is aborted and recorded, never + resolved.""" + def step() -> Optional[str]: + merge = run_registered( + ["git", "merge", "--no-edit", f"origin/{source}"], + cwd=str(home_dir), timeout=60, + ) + if merge.returncode == 0: + return None + run_registered(["git", "merge", "--abort"], cwd=str(home_dir), timeout=30) + return f"merging {source}: " + _summarize_git_error( + merge.stderr or merge.stdout or "merge failed") + + return _with_stash(home_dir, branch, step) def _run_pull_once(home_dir: Path) -> Dict: """One pull cycle (trinity-enterprise#703, invariant G3): bring origin's - commits for the checked-out branch to the agent. + commits to the agent. - Source-mode agents are checked out on their source branch and working-branch - agents on their working branch, so the checked-out branch is the one to pull. + Source-mode agents are checked out on their source branch, so that branch is + pulled. A working-branch agent (`trinity/*`) is checked out on its own + branch, which only it writes — so its own branch is brought up to date AND + the source branch (`_get_pull_branch`, normally `main`) is merged in, or + human work pushed to `main` would never arrive (PR #3021 ruling). - - Never while an execution runs (a rebase under a running turn changes the - files it is reading) — checked before the fetch and again right before the - tree is touched; an unreadable registry counts as busy. + - Never while an execution runs or is queued (a rebase under a turn changes + the files it is reading) — checked before the fetch and again right before + the tree is touched; an unreadable registry counts as busy. - Serialised with the push cycle and the operator git endpoints on - `_REPO_LOCK`; a busy repo skips quietly. - - `_integrate_remote`: a fast-forward when nothing is committed locally, a - rebase (aborted on conflict) when something is; uncommitted edits are - stashed explicitly and re-applied, and a pull whose incoming changes - collide with them is undone. Local work is never discarded; a failure is - recorded, not resolved. + `_REPO_LOCK`; a busy repo skips quietly (and unrecorded: every write to + sync-state.json happens under that lock). + - Stale lock litter is reaped first, as the push cycle does: a pull-only + agent has no push cycle to do it. + - Refuses to run over unmerged paths. Local work is never discarded; a + failure is recorded, not resolved. + + `behind_after_pull` is the checked-out branch's own lag behind + `origin/`, not its lag behind `main`; `behind_main` in the git + status is the latter. """ - # The lock first: every write to sync-state.json happens under it, because - # the push cycle read-modify-writes the same file from another thread. if not _REPO_LOCK.acquire(blocking=False): return {"status": "skipped", "reason": "repo_busy"} try: + _reap_stale_git_litter( + home_dir, repack_budget_seconds=_maintenance_timeout_seconds() + ) running = _executions_in_flight() if running is None or running > 0: return _record_pull(home_dir, "skipped", error=( @@ -937,26 +1035,55 @@ def _run_pull_once(home_dir: Path) -> Dict: ).stdout.strip() if not branch: return _record_pull(home_dir, "failed", error="detached HEAD: nothing to pull onto") + unmerged = _unmerged_paths(home_dir) + if unmerged: + return _record_pull(home_dir, "failed", error=( + f"unmerged paths in the working tree ({', '.join(unmerged[:3])}): " + "resolve them before the pull can run")) + source = _get_pull_branch(branch, home_dir) + refs = [branch] if source == branch else [branch, source] fetch = run_registered( - ["git", "fetch", "origin", branch], cwd=str(home_dir), timeout=120, + ["git", "fetch", "origin", *refs], cwd=str(home_dir), timeout=120, ) + own_on_origin = True if fetch.returncode != 0: - if _is_missing_remote_ref(fetch.stderr): - return _record_pull(home_dir, "success", behind=0) # not on origin yet - return _record_pull(home_dir, "failed", error=_summarize_git_error( - fetch.stderr or fetch.stdout or "fetch failed")) - ahead, behind = _compute_ahead_behind(home_dir, branch) - if behind == 0: + if not _is_missing_remote_ref(fetch.stderr): + return _record_pull(home_dir, "failed", error=_summarize_git_error( + fetch.stderr or fetch.stdout or "fetch failed")) + # The working branch is not on origin yet (its first push has not + # happened): the source branch can still come in. + own_on_origin = False + if source == branch: + return _record_pull(home_dir, "success", behind=0) + retry = run_registered( + ["git", "fetch", "origin", source], cwd=str(home_dir), timeout=120, + ) + if retry.returncode != 0: + return _record_pull(home_dir, "failed", error=_summarize_git_error( + retry.stderr or retry.stdout or "fetch failed")) + + ahead, behind = _compute_ahead_behind(home_dir, branch) if own_on_origin else (0, 0) + source_behind = 0 + if source != branch: + _, source_behind = _compute_ahead_behind(home_dir, source) + if behind == 0 and source_behind == 0: return _record_pull(home_dir, "success", behind=0) + running = _executions_in_flight() if running is None or running > 0: return _record_pull(home_dir, "skipped", behind=behind, error=( "execution in flight" if running else "execution state unknown")) - err = _integrate_remote(home_dir, branch, ahead) - if err: - return _record_pull(home_dir, "failed", behind=behind, error=err) - _, after = _compute_ahead_behind(home_dir, branch) - logger.info("pull: %s brought in %s commit(s) on %s", home_dir, behind, branch) + if behind: + err = _integrate_remote(home_dir, branch, ahead) + if err: + return _record_pull(home_dir, "failed", behind=behind, error=err) + if source_behind: + err = _integrate_source(home_dir, branch, source) + if err: + return _record_pull(home_dir, "failed", behind=0, error=err) + after = _compute_ahead_behind(home_dir, branch)[1] if own_on_origin else 0 + logger.info("pull: %s brought in %s commit(s) on %s and %s from %s", + home_dir, behind, branch, source_behind, source) return _record_pull(home_dir, "success", behind=after) except subprocess.CalledProcessError as exc: return _record_pull(home_dir, "failed", error=_summarize_git_error( @@ -1026,6 +1153,14 @@ def _fail(err: str, behind: Optional[int] = None) -> Dict: if _is_shared_source_branch(home_dir, branch): return _fail(f"refused: source-mode on {branch}") + # Never stage over unmerged paths: `git add -A` resolves a `UU` + # file by staging its conflict markers, and the push below would + # carry them to origin as a successful sync (PR #3021 review). + unmerged = _unmerged_paths(home_dir) + if unmerged: + return _fail( + f"refused: unmerged paths ({', '.join(unmerged[:3])})") + # Stage everything. run_registered( ["git", "add", "-A"], cwd=str(home_dir), timeout=30, check=True, diff --git a/tests/unit/test_ent703_pull_cycle.py b/tests/unit/test_ent703_pull_cycle.py index 0e61015ffd..7f2d60855e 100644 --- a/tests/unit/test_ent703_pull_cycle.py +++ b/tests/unit/test_ent703_pull_cycle.py @@ -207,6 +207,191 @@ def test_up_to_date_and_unpushed_branch_are_quiet_successes(self, world): assert _state(agent)["behind_after_pull"] == 0 +class TestReviewFixes: + """PR #3021 review: every exit keeps local work, a failed undo stops the + push, queued turns count, and working-branch agents receive `main`.""" + + @staticmethod + def _timeout_on(monkeypatch, verb, times=1): + """`run_registered` raises TimeoutExpired for the next `times` git `verb`s.""" + real = git_router.run_registered + left = {"n": times} + + def fake(cmd, *a, **kw): + if len(cmd) > 1 and cmd[1] == verb and left["n"] > 0: + left["n"] -= 1 + raise subprocess.TimeoutExpired(cmd, kw.get("timeout", 1)) + return real(cmd, *a, **kw) + + monkeypatch.setattr(git_router, "run_registered", fake) + + def test_a_timed_out_fast_forward_puts_the_edits_back(self, world, monkeypatch): + agent, _, human = world + before = _out(agent, "rev-parse", "HEAD") + _human_push(human, "role.md", "role v2\n") + (agent / "notes.md").write_text("notes — uncommitted\n") + self._timeout_on(monkeypatch, "merge") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert result["error"] == "git merge timed out" + assert _out(agent, "rev-parse", "HEAD") == before + assert (agent / "notes.md").read_text() == "notes — uncommitted\n" + assert _out(agent, "stash", "list") == "" + + def test_a_stash_left_behind_is_named_in_the_error(self, world, monkeypatch): + agent, _, human = world + _human_push(human, "role.md", "role v2\n") + (agent / "notes.md").write_text("notes — uncommitted\n") + # the ff times out, then every restore attempt's pop times out too + self._timeout_on(monkeypatch, "merge") + real = git_router.run_registered + + def no_pop(cmd, *a, **kw): + if cmd[1:3] == ["stash", "pop"]: + raise subprocess.TimeoutExpired(cmd, 1) + return real(cmd, *a, **kw) + + monkeypatch.setattr(git_router, "run_registered", no_pop) + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert "local edits are kept in `git stash`" in result["error"] + assert "trinity-pull" in _out(agent, "stash", "list") + + def test_a_failed_undo_stops_loudly_and_the_push_refuses_the_markers(self, world, monkeypatch): + agent, origin, human = world + _human_push(human, "notes.md", "notes — human rewrite\n") + (agent / "notes.md").write_text("notes — agent's uncommitted edit\n") + real = git_router.run_registered + + def reset_fails(cmd, *a, **kw): + if cmd[1:3] == ["reset", "--hard"]: + return subprocess.CompletedProcess(cmd, 128, "", "fatal: Unable to create '.git/index.lock': File exists.") + return real(cmd, *a, **kw) + + monkeypatch.setattr(git_router, "run_registered", reset_fails) + result = git_router._run_pull_once(agent) + monkeypatch.setattr(git_router, "run_registered", real) + + assert result["status"] == "failed" + assert "could not be undone" in result["error"] + assert "local edits are kept in `git stash`" in result["error"] + assert git_router._unmerged_paths(agent) == ["notes.md"] + + origin_main = _out(origin, "rev-parse", "main") + push = git_router._run_auto_sync_once(agent) + assert push["status"] == "failed" + assert push["error"].startswith("refused: unmerged paths (notes.md)") + assert _out(origin, "rev-parse", "main") == origin_main + assert "<<<<<<<" not in _out(origin, "show", "main:notes.md") + + def test_a_pull_refuses_to_run_over_unmerged_paths(self, world, monkeypatch): + agent, _, human = world + monkeypatch.setattr(git_router, "_unmerged_paths", lambda home: ["notes.md"]) + _human_push(human, "role.md", "role v2\n") + before = _out(agent, "rev-parse", "HEAD") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" and "unmerged paths" in result["error"] + assert _out(agent, "rev-parse", "HEAD") == before + + def test_a_queued_turn_counts_as_busy(self, monkeypatch): + from agent_server.services import process_registry as pr + + class Reg: + def list_running(self): + return [] + + def list_pending_ids(self): + return ["exec-queued"] + + monkeypatch.setattr(pr, "get_process_registry", lambda: Reg()) + assert git_router._executions_in_flight() == 1 + + def test_a_stale_index_lock_is_reaped_before_the_pull(self, world): + import os + import time + agent, _, human = world + sha = _human_push(human, "role.md", "role v2\n") + lock = agent / ".git" / "index.lock" + lock.write_text("") + old = time.time() - 7200 + os.utime(lock, (old, old)) + + assert git_router._run_pull_once(agent)["status"] == "success" + assert _out(agent, "rev-parse", "HEAD") == sha + assert not lock.exists() + + def test_streaks_and_last_success_are_recorded(self, world, monkeypatch): + agent, _, human = world + monkeypatch.setattr(git_router, "_executions_in_flight", lambda: 1) + git_router._run_pull_once(agent) + git_router._run_pull_once(agent) + st = _state(agent) + assert (st["consecutive_pull_skips"], st["consecutive_pull_failures"]) == (2, 0) + assert st.get("last_successful_pull_at") is None + + monkeypatch.setattr(git_router, "_executions_in_flight", lambda: 0) + _human_push(human, "notes.md", "notes — human rewrite\n") + (agent / "notes.md").write_text("notes — agent's uncommitted edit\n") + git_router._run_pull_once(agent) + st = _state(agent) + assert (st["consecutive_pull_skips"], st["consecutive_pull_failures"]) == (0, 1) + assert st["last_pull_error"] == "local edits conflict with incoming changes on main" + + _git(agent, "checkout", "--", "notes.md") + git_router._run_pull_once(agent) + st = _state(agent) + assert st["consecutive_pull_failures"] == 0 and st["last_pull_error"] is None + assert st["last_successful_pull_at"] == st["last_pull_at"] + + +class TestWorkingBranchReceivesMain: + """PR #3021 ruling: a `trinity/*` agent only ever pulled its own branch, so + human work pushed to `main` never arrived (invariant G3).""" + + @staticmethod + def _on_working_branch(agent): + _git(agent, "checkout", "-q", "-b", "trinity/agent/1") + (agent / "notes.md").write_text("notes — the agent's\n") + _git(agent, "commit", "-qam", "agent: notes") + _git(agent, "push", "-q", "-u", "origin", "trinity/agent/1") + + def test_main_is_merged_into_the_working_branch(self, world): + agent, origin, human = world + self._on_working_branch(agent) + pushed = _out(agent, "rev-parse", "HEAD") + sha = _human_push(human, "role.md", "role v2 — from main\n") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "success" + assert _out(agent, "symbolic-ref", "--short", "HEAD") == "trinity/agent/1" + assert (agent / "role.md").read_text() == "role v2 — from main\n" + assert (agent / "notes.md").read_text() == "notes — the agent's\n" + # a merge, not a rewrite: the pushed working branch is still an ancestor + assert _git(agent, "merge-base", "--is-ancestor", pushed, "HEAD", check=False).returncode == 0 + assert _git(agent, "merge-base", "--is-ancestor", sha, "HEAD", check=False).returncode == 0 + + def test_a_conflicting_main_is_aborted_and_recorded(self, world): + agent, _, human = world + self._on_working_branch(agent) + before = _out(agent, "rev-parse", "HEAD") + _human_push(human, "notes.md", "notes — human\n") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert result["error"].startswith("merging main: ") + assert _out(agent, "rev-parse", "HEAD") == before + assert not (agent / ".git" / "MERGE_HEAD").exists() + assert (agent / "notes.md").read_text() == "notes — the agent's\n" + + class TestLoopGate: @pytest.fixture(autouse=True) def _env(self, monkeypatch): From 8c1282456548a6005cb982609d4de44a436b741f Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Mon, 28 Sep 2026 11:06:42 +0300 Subject: [PATCH 4/8] feat(git-sync): persist pull health; renumber to 0080_pull_sync; docs + UI copy (trinity-enterprise#703) - Alembic 0076_pull_sync -> 0080_pull_sync <- 0079_telegram_group_context (one head, 81 revisions); SQLite entry re-appended after dev's. - The same migration pair adds agent_sync_state.last_pull_error, last_successful_pull_at, consecutive_pull_failures and consecutive_pull_skips, persisted by the sync-health poller (bounded, coerced; a success clears the error). ent#706/#707 read these, so they land here rather than as a second migration on the same table. - agent-runtime.md says three loops; the flow and requirement describe the main merge, the queued-turn gate, the unmerged-path refusal, what behind_after_pull measures, and that auto-sync agents already rebase in the push cycle. - Settings copy no longer hard-codes 15 minutes (GIT_SYNC_PULL_INTERVAL_SECONDS) and no longer claims the push cycle never touches the tree while the agent works. Co-Authored-By: Claude Opus 5.5 --- docs/memory/architecture/agent-runtime.md | 2 +- docs/memory/feature-flows/git-sync-health.md | 36 +++++++++++++++---- docs/memory/requirements/github.md | 14 +++++--- src/backend/db/migrations.py | 12 +++++-- src/backend/db/schema.py | 4 +++ src/backend/db/sync_state.py | 15 ++++++++ src/backend/db/tables.py | 4 +++ .../{0076_pull_sync.py => 0080_pull_sync.py} | 27 ++++++++++---- src/backend/services/sync_health_service.py | 4 +++ .../src/components/GitSyncSettingsPanel.vue | 8 +++-- tests/unit/test_ent703_pull_cycle.py | 4 +-- tests/unit/test_sync_health_service.py | 34 ++++++++++++++++++ 12 files changed, 139 insertions(+), 25 deletions(-) rename src/backend/migrations/versions/{0076_pull_sync.py => 0080_pull_sync.py} (56%) diff --git a/docs/memory/architecture/agent-runtime.md b/docs/memory/architecture/agent-runtime.md index 920b01f42a..a28b4649c2 100644 --- a/docs/memory/architecture/agent-runtime.md +++ b/docs/memory/architecture/agent-runtime.md @@ -24,7 +24,7 @@ - `/api/files`, `/api/files/download` (100MB limit), `/api/files/mkdir` (workspace-confined, #37) - `/api/brain-orb/data` - Streams the agent's `resources/agent-visualization/data.json` (Brain Orb read surface, #58); 404 when absent. `/api/brain-orb/scopes` + `/api/brain-orb/scope` run the agent's `~/.trinity/brain-orb/{scopes,scope}` convention hooks for live scope control (#58 Phase 2); `/api/brain-orb/tool` runs the read-only `~/.trinity/brain-orb/search` hook (#60 Phase 3) -The agent server also runs two loops: the 15-min git `auto_sync` heartbeat (see [Git Sync Health](agent-lifecycle.md#git-sync-health-389390)) and the 5s liveness heartbeat (see [Heartbeat Liveness](reliability.md#heartbeat-liveness-reliability-004-307)). `GET /api/git/status` is not a loop but is driven like one (backend poll every 60 s, plus the UI git panel and the MCP tool): since #2742 it is single-flighted on the event loop, computed in one `to_thread` worker, sweep-registered, and takes **no** `.git/index.lock` — see the same section. +The agent server also runs three loops: the 15-min git `auto_sync` push heartbeat, the git pull cycle beside it (trinity-enterprise#703, `GIT_SYNC_PULL_INTERVAL_SECONDS`, defaulting to the push interval; both in [Git Sync Health](agent-lifecycle.md#git-sync-health-389390)), and the 5s liveness heartbeat (see [Heartbeat Liveness](reliability.md#heartbeat-liveness-reliability-004-307)). `GET /api/git/status` is not a loop but is driven like one (backend poll every 60 s, plus the UI git panel and the MCP tool): since #2742 it is single-flighted on the event loop, computed in one `to_thread` worker, sweep-registered, and takes **no** `.git/index.lock` — see the same section. **Execution environment assembly (#1999):** a spawned runtime subprocess no longer inherits the agent-server's long-lived `os.environ`. `agent_server/services/execution_env.py::build_execution_env` rebuilds it per spawn from three inspectable inputs — `INITIAL_ENV` (the container baseline captured at import, i.e. what `docker exec` shows) → `~/.env` parsed fresh (**authoritative for credentials**, so a removed key is removed) → `RUNTIME_OVERRIDES` (values that are deliberately not `.env` credentials; the #1089 token rotation and the #2114 subscription-shadow arm, applied AFTER the file so a stale token in `.env` can't beat an explicit rotation, with `None` meaning force-unset) → the caller's `extra` (last, so `EXECUTION_TAG_NAME` #407 can't be displaced). All five spawn sites (`claude_code`, `headless_executor`, `codex_runtime`, `gemini_runtime` ×2) route through it. The credential endpoints still mirror `.env` into `os.environ` for **in-process** readers (error classifier, `AGENT_RUNTIME`, the sanitizer's redaction set) but now with a **delete phase** that restores the container baseline rather than popping blind, and only for keys the mirror itself wrote. `.env` may not set loader/exec-redirecting names (`PATH`, `LD_PRELOAD`, `BASH_ENV`, …) — it is agent-writable and is now read at every spawn. **ent#615 widened that set and added a prefix rule** (`is_protected_key`): `GIT_CONFIG_COUNT` was listed but its SLOTS (`GIT_CONFIG_KEY_*` / `GIT_CONFIG_VALUE_*`, an unbounded family) were not, so the guard covered the count and left the payload writable — and a slot can set `core.sshCommand`, `core.pager`, `diff.external` or `credential.helper`, i.e. the same execution vectors by another route. `GIT_CONFIG_NOSYSTEM`, `GIT_ASKPASS` and `GIT_PROXY_COMMAND` join them; `NOSYSTEM` is load-bearing because `/etc/gitconfig` is where ent#615 registers the git credential helper, alongside the #1595 gc guards, so one `.env` line would disable both — and a helper that stops running is a fetch/push outage with no error that names it. `GET /api/credentials/status` reports per-key `env_drift` (**names only, never values**). Before this, the file and the execution environment were independent channels that synced one-way with no delete phase, so a key removed from `.env` outside the credentials API kept reaching every execution until container restart — invisible to both `/proc//environ` and `docker exec`, i.e. silent failure of credential revocation. **`.env` is deliberately NOT authoritative for API-key-style Claude auth while subscription auth is active (#2114):** #1999's re-read made a stale `.env` `ANTHROPIC_API_KEY` (preferred by Claude Code over the OAuth token, and never cleaned by any recreate — it lives on the workspace volume) shadow subscription auth at every spawn, which SUB-003 then mis-attributed to each healthy subscription in turn. At boot, `arm_subscription_auth_guard()` force-unsets `ANTHROPIC_API_KEY` + `ANTHROPIC_AUTH_TOKEN` through the same override layer when the baseline carries a truthy `CLAUDE_CODE_OAUTH_TOKEN` on a Claude runtime — restart-durable because `startup.sh` exports the rotated override token *before* the server launches, so it is always baseline; non-Claude runtimes never arm (a vestigial token must not strip a key their scripts may use). The suppression is **data, not just a memoized per-key WARNING**: `env_drift_report` marks force-unset keys `suppressed_for_spawn` (iteration set includes override-only keys), so the one surface built to expose file/spawn divergence cannot show all-green over an active suppression. diff --git a/docs/memory/feature-flows/git-sync-health.md b/docs/memory/feature-flows/git-sync-health.md index 933862134c..64c82b1692 100644 --- a/docs/memory/feature-flows/git-sync-health.md +++ b/docs/memory/feature-flows/git-sync-health.md @@ -372,27 +372,51 @@ SessionStart hook, and the 2026-09-24 audit found agents up to 31 commits behind - existing agents: on only where `auto_sync_enabled` was already on - everyone else: off until toggled in Settings → Git sync - **Cycle** (`_run_pull_once`, under `_REPO_LOCK`): - - Skipped while any execution is running, checked again right before the - tree is touched; an unreadable registry counts as busy. + - Stale lock litter is reaped first (`_reap_stale_git_litter`), as the push + cycle does — a pull-only agent has no push cycle to do it. + - Skipped while any execution is running **or queued** (`list_running()` + + `list_pending_ids()`, #2433), checked again right before the tree is + touched; an unreadable registry counts as busy. + - Refused while the index holds unmerged paths. - Fetches the **checked-out** branch: source-mode agents sit on their source branch, working-branch agents on theirs. - Nothing committed locally → a fast-forward. A local commit → a rebase, aborted on conflict (`diverged: …`). + - A `trinity/*` working branch also gets its source branch + (`_get_pull_branch`, `main`) **merged** in (`_integrate_source`) — only the + agent writes its working branch, so without this human pushes to `main` + never arrive. A merge, not a rebase: the branch is already pushed. A + conflict is aborted and recorded as `merging main: …`. - **Uncommitted edits** (`_integrate_remote`) are stashed explicitly and re-applied. `--autostash` is deliberately not used: when the incoming commits touch the same files, its re-apply conflicts, leaves the edits *only in the stash*, and still reports success. Instead a colliding pull is **undone**: back to the pre-pull HEAD, where the stash applies cleanly. The result is recorded as `local edits conflict with incoming changes on `. + Every exit (`_with_stash`) — a git child timing out included — either puts + the edits back or says `local edits are kept in \`git stash\``; a failed + `reset --hard` stops there and says so. The push cycle refuses to stage over + unmerged paths, so conflict markers left by such a failure are never + committed and pushed. - **Recorded:** - `sync-state.json`: `last_pull_at`, `last_pull_status` - (`success`/`failed`/`skipped`), `last_pull_error`, `behind_after_pull`. + (`success`/`failed`/`skipped`), `last_pull_error`, `behind_after_pull`, + `last_successful_pull_at`, `consecutive_pull_failures`, + `consecutive_pull_skips`. A repo-busy skip is not recorded (every write to + this file happens under the lock it could not take). + `behind_after_pull` is the checked-out branch's lag behind + `origin/` — on a `trinity/*` agent that is its own branch, not + `main`; the git status's `behind_main` is the latter. These are written through `_patch_sync_state`, so the push's `consecutive_failures` / `last_sync_*` are never touched. - The poller persists them on `agent_sync_state` (status bounded to the - vocabulary, counter coerced). -- **Not here:** merging the source branch into a working branch on a longer - interval (noted in the issue); the hooks-vs-heartbeat owner (ent#708). + vocabulary, counters coerced, error capped at 500 chars). Nothing feeds + `sync_failing` from them yet — that is ent#706/#707. +- **Auto-sync agents already reconcile:** the #3011 push cycle fetches and + rebases onto `origin/` every cycle, without an execution gate. For + them the pull's own-branch step is mostly redundant; what it adds is the + execution gate on its own step and `main` for working branches. +- **Not here:** the hooks-vs-heartbeat owner (ent#708). ### 2. Backend poller diff --git a/docs/memory/requirements/github.md b/docs/memory/requirements/github.md index 9d0e38decc..1de89597c5 100644 --- a/docs/memory/requirements/github.md +++ b/docs/memory/requirements/github.md @@ -864,17 +864,23 @@ - **Invariant G3:** human and fleet work reaches the agent within a bound. - **Pull cycle** in the agent server beside the push cycle: - on its interval: fetch, then fast-forward or rebase + - a `trinity/*` working branch also merges `main` in, so human pushes to + `main` arrive (a merge — the branch is already pushed; conflict aborted) - uncommitted edits stashed and re-applied explicitly - - never discards local work: a conflict is aborted or undone, and recorded - - never runs while an execution is in flight + - never discards local work: a conflict is aborted or undone, and recorded; + an edit that could not be put back is named as kept in `git stash` + - never runs while an execution is in flight or queued, nor over unmerged + paths (and the push cycle never commits them) - **Flag:** per-agent `pull_sync_enabled` (both migration tracks), read live each cycle, with `GIT_SYNC_PULL` as the fallback; interval `GIT_SYNC_PULL_INTERVAL_SECONDS` (defaults to the push interval). - on for new `github:` agents, source-mode included - on for existing agents only where auto-sync is already on - off otherwise; toggle in Settings → Git sync -- **Observability:** `last_pull_at`, `last_pull_status`, `behind_after_pull` - in `sync-state.json`, persisted on `agent_sync_state`. A failed pull never +- **Observability:** `last_pull_at`, `last_pull_status`, `last_pull_error`, + `behind_after_pull`, `last_successful_pull_at`, `consecutive_pull_failures`, + `consecutive_pull_skips` in `sync-state.json`, persisted on + `agent_sync_state`. A failed pull never counts toward the push's `consecutive_failures`. - **Edition**: open-core (operator ruling 2026-09-25). - **Flow**: `docs/memory/feature-flows/git-sync-health.md` §1d diff --git a/src/backend/db/migrations.py b/src/backend/db/migrations.py index 6d90d74cd5..b3ee504ca0 100644 --- a/src/backend/db/migrations.py +++ b/src/backend/db/migrations.py @@ -4833,15 +4833,17 @@ def _migrate_pull_sync(cursor, conn): * `agent_git_config.pull_sync_enabled` — the per-agent switch the agent's pull loop reads live each cycle (the #3010 one-writer discipline). - * `agent_sync_state.last_pull_at / last_pull_status / behind_after_pull` — - the pull cycle's own outcome, persisted by the sync-health poller. + * `agent_sync_state.last_pull_at / last_pull_status / behind_after_pull / + last_pull_error / last_successful_pull_at / consecutive_pull_failures / + consecutive_pull_skips` — the pull cycle's own outcome and health, + persisted by the sync-health poller. Backfill (operator ruling 2026-09-25): on only where auto-sync is already on, so no agent that is not already writing to git starts rebasing its working tree on upgrade; everyone else is off until toggled. New `github:` agents get it at creation. Runs once (schema_migrations). - Mirrored by the Alembic revision 0076_pull_sync. + Mirrored by the Alembic revision 0080_pull_sync. """ _safe_add_column( cursor, "agent_git_config", "pull_sync_enabled", @@ -4851,6 +4853,10 @@ def _migrate_pull_sync(cursor, conn): ("last_pull_at", "TEXT"), ("last_pull_status", "TEXT"), ("behind_after_pull", "INTEGER"), + ("last_pull_error", "TEXT"), + ("last_successful_pull_at", "TEXT"), + ("consecutive_pull_failures", "INTEGER DEFAULT 0"), + ("consecutive_pull_skips", "INTEGER DEFAULT 0"), ): _safe_add_column( cursor, "agent_sync_state", column, diff --git a/src/backend/db/schema.py b/src/backend/db/schema.py index f7cc35d10f..6043c7ad7c 100644 --- a/src/backend/db/schema.py +++ b/src/backend/db/schema.py @@ -1201,6 +1201,10 @@ last_pull_at TEXT, -- trinity-enterprise#703: the container's pull cycle last_pull_status TEXT, behind_after_pull INTEGER, + last_pull_error TEXT, + last_successful_pull_at TEXT, + consecutive_pull_failures INTEGER DEFAULT 0, + consecutive_pull_skips INTEGER DEFAULT 0, last_check_at TEXT, updated_at TEXT NOT NULL, FOREIGN KEY (agent_name) REFERENCES agent_ownership(agent_name) diff --git a/src/backend/db/sync_state.py b/src/backend/db/sync_state.py index 0d3d3e0c15..39115f52e3 100644 --- a/src/backend/db/sync_state.py +++ b/src/backend/db/sync_state.py @@ -38,6 +38,10 @@ "last_pull_at", # trinity-enterprise#703: the container's pull cycle "last_pull_status", "behind_after_pull", + "last_pull_error", + "last_successful_pull_at", + "consecutive_pull_failures", + "consecutive_pull_skips", "last_check_at", "updated_at", ) @@ -86,6 +90,10 @@ def upsert( last_pull_at: Optional[str] = None, last_pull_status: Optional[str] = None, behind_after_pull: Optional[int] = None, + last_pull_error: Optional[str] = None, + last_successful_pull_at: Optional[str] = None, + consecutive_pull_failures: Optional[int] = None, + consecutive_pull_skips: Optional[int] = None, last_check_at: Optional[str] = None, ) -> Dict: """Upsert a sync-state row. @@ -135,6 +143,13 @@ def _merged(field: str, new_value): "last_pull_at": _merged("last_pull_at", last_pull_at), "last_pull_status": _merged("last_pull_status", last_pull_status), "behind_after_pull": _merged("behind_after_pull", behind_after_pull), + # last_pull_error is written as-is, None included: a success clears + # it, so a stale error never outlives the pull that fixed it. + "last_pull_error": last_pull_error if last_pull_status is not None + else _merged("last_pull_error", None), + "last_successful_pull_at": _merged("last_successful_pull_at", last_successful_pull_at), + "consecutive_pull_failures": _merged("consecutive_pull_failures", consecutive_pull_failures) or 0, + "consecutive_pull_skips": _merged("consecutive_pull_skips", consecutive_pull_skips) or 0, "last_check_at": last_check_at or now, "updated_at": now, } diff --git a/src/backend/db/tables.py b/src/backend/db/tables.py index 426253f196..96ca6a064d 100644 --- a/src/backend/db/tables.py +++ b/src/backend/db/tables.py @@ -1021,6 +1021,10 @@ def process_bind_param(self, value, dialect): Column("last_pull_at", Text), # trinity-enterprise#703: the container's pull cycle Column("last_pull_status", Text), Column("behind_after_pull", Integer), + Column("last_pull_error", Text), + Column("last_successful_pull_at", Text), + Column("consecutive_pull_failures", Integer), + Column("consecutive_pull_skips", Integer), Column("last_check_at", Text), Column("updated_at", Text), ) diff --git a/src/backend/migrations/versions/0076_pull_sync.py b/src/backend/migrations/versions/0080_pull_sync.py similarity index 56% rename from src/backend/migrations/versions/0076_pull_sync.py rename to src/backend/migrations/versions/0080_pull_sync.py index fd42dfa66d..6fce4485c0 100644 --- a/src/backend/migrations/versions/0076_pull_sync.py +++ b/src/backend/migrations/versions/0080_pull_sync.py @@ -4,18 +4,19 @@ ``db/migrations.py::pull_sync``. Adds ``agent_git_config.pull_sync_enabled`` (the per-agent switch) and -``agent_sync_state.last_pull_at / last_pull_status / behind_after_pull`` (the -pull cycle's outcome), then turns the pull on only where auto-sync is already +``agent_sync_state.last_pull_at / last_pull_status / behind_after_pull / +last_pull_error / last_successful_pull_at / consecutive_pull_failures / +consecutive_pull_skips`` (the pull cycle's outcome and health), then turns the pull on only where auto-sync is already on (operator ruling 2026-09-25). -Revision ID: 0076_pull_sync -Revises: 0075_auto_sync_enabled_backfill +Revision ID: 0080_pull_sync +Revises: 0079_telegram_group_context """ from alembic import op -revision = "0076_pull_sync" -down_revision = "0075_auto_sync_enabled_backfill" +revision = "0080_pull_sync" +down_revision = "0079_telegram_group_context" branch_labels = None depends_on = None @@ -30,6 +31,16 @@ def upgrade() -> None: op.execute("ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS last_pull_at TEXT") op.execute("ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS last_pull_status TEXT") op.execute("ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS behind_after_pull INTEGER") + op.execute("ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS last_pull_error TEXT") + op.execute("ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS last_successful_pull_at TEXT") + op.execute( + "ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS " + "consecutive_pull_failures INTEGER DEFAULT 0" + ) + op.execute( + "ALTER TABLE agent_sync_state ADD COLUMN IF NOT EXISTS " + "consecutive_pull_skips INTEGER DEFAULT 0" + ) op.execute( "UPDATE agent_git_config SET pull_sync_enabled = 1 " "WHERE COALESCE(auto_sync_enabled, 0) = 1" @@ -37,6 +48,10 @@ def upgrade() -> None: def downgrade() -> None: + op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS consecutive_pull_skips") + op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS consecutive_pull_failures") + op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS last_successful_pull_at") + op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS last_pull_error") op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS behind_after_pull") op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS last_pull_status") op.execute("ALTER TABLE agent_sync_state DROP COLUMN IF EXISTS last_pull_at") diff --git a/src/backend/services/sync_health_service.py b/src/backend/services/sync_health_service.py index c4df81e488..1fc4c584fd 100644 --- a/src/backend/services/sync_health_service.py +++ b/src/backend/services/sync_health_service.py @@ -472,6 +472,10 @@ async def _sync_agent(self, config) -> None: last_pull_at=_pull_text(sync_state.get("last_pull_at"), 64), last_pull_status=_pull_status(sync_state.get("last_pull_status")), behind_after_pull=_coerce_nonneg_int(sync_state.get("behind_after_pull")), + last_pull_error=_pull_text(sync_state.get("last_pull_error"), 500), + last_successful_pull_at=_pull_text(sync_state.get("last_successful_pull_at"), 64), + consecutive_pull_failures=_coerce_nonneg_int(sync_state.get("consecutive_pull_failures")), + consecutive_pull_skips=_coerce_nonneg_int(sync_state.get("consecutive_pull_skips")), last_check_at=utc_now_iso(), ) diff --git a/src/frontend/src/components/GitSyncSettingsPanel.vue b/src/frontend/src/components/GitSyncSettingsPanel.vue index 040d4aa7f2..a0b43d61af 100644 --- a/src/frontend/src/components/GitSyncSettingsPanel.vue +++ b/src/frontend/src/components/GitSyncSettingsPanel.vue @@ -66,13 +66,15 @@

- Brings in edits people and other agents push to the repository. Never - runs while the agent is working, and never discards its own changes. + Brings in edits people and other agents push to the repository, and + merges main into a working branch. The pull never runs while + the agent is working or has a turn queued, and never discards its own + changes. With auto-sync on, each push also rebases onto GitHub first.

Date: Tue, 29 Sep 2026 10:21:55 +0300 Subject: [PATCH 5/8] fix(git-sync): pull records merge conflicts as conflicts; rebases keep merges (trinity-enterprise#703) PR #3021 re-review: - A conflict merging main was recorded as "Auto-merging ": git prints it on stdout with an empty stderr. Unmerged paths are read before the abort; the error is "diverged: merge conflict with main ()", and a failed merge --abort is named. - `git rebase` flattened the merge the pull made into copies of main's commits; both cycles now rebase with --rebase-merges. - A failed strict ahead/behind count fails the pull instead of reading as up to date. - A timed-out step is reset to the pre-pull HEAD even with nothing stashed; a reset blocked by the killed child's index.lock says so. - "Never runs while the agent is working" is check-then-act: the copy and docs now say "never starts" and document the admission window. Co-Authored-By: Claude Opus 5.5 --- docker/base-image/agent_server/routers/git.py | 78 +++++++++--- docs/memory/architecture/agent-lifecycle.md | 2 +- docs/memory/feature-flows/git-sync-health.md | 19 ++- docs/memory/requirements/github.md | 8 +- .../src/components/GitSyncSettingsPanel.vue | 4 +- tests/unit/test_ent703_pull_cycle.py | 116 +++++++++++++++++- 6 files changed, 200 insertions(+), 27 deletions(-) diff --git a/docker/base-image/agent_server/routers/git.py b/docker/base-image/agent_server/routers/git.py index 891a384a84..fe7b8e0631 100644 --- a/docker/base-image/agent_server/routers/git.py +++ b/docker/base-image/agent_server/routers/git.py @@ -797,10 +797,16 @@ def _rebase_onto_remote(home_dir: Path, branch: str) -> Optional[str]: Returns None on a clean rebase, else the error summary to record. Any failure is aborted, so the repo is back exactly where it was — a conflict is never resolved automatically, the remote never overwritten, nothing reset. + + `--rebase-merges` (PR #3021 re-review): the pull cycle MERGES `main` into a + working branch, and a plain rebase drops merge commits — it replayed `main`'s + commits as copies, so `origin/main` stopped being an ancestor and the next + cycle merged `main` again on top of the duplicates. On a linear history the + flag changes nothing. """ try: rebase = run_registered( - ["git", "rebase", "--autostash", f"origin/{branch}"], + ["git", "rebase", "--autostash", "--rebase-merges", f"origin/{branch}"], cwd=str(home_dir), timeout=120, ) except subprocess.TimeoutExpired: @@ -946,14 +952,20 @@ def git(*args, timeout=60): except subprocess.TimeoutExpired as exc: verb = (exc.cmd or ["git", "command"])[1:2] or ["command"] err = f"git {verb[0]} timed out" - if stashed: - # Best effort back to the base the stash was taken on, then re-apply. - try: - if git("reset", "--hard", pre_head, timeout=30).returncode == 0 \ - and git("stash", "pop", timeout=30).returncode == 0: - stashed = False - except subprocess.TimeoutExpired: - pass + # Back to the pre-pull HEAD whether or not anything was stashed: a + # killed merge can leave a half-updated tree or a MERGE_HEAD that the + # next push cycle would commit as the agent's own work (PR #3021 + # re-review). Then re-apply the stash on the base it was taken from. + try: + git("merge", "--abort", timeout=30) # no-op unless MERGE_HEAD exists + reset = git("reset", "--hard", pre_head, timeout=30) + if reset.returncode != 0: + err += (f"; the tree could not be reset to {pre_head[:12]} (" + f"{_summarize_git_error(reset.stderr or reset.stdout or 'reset failed')})") + elif stashed and git("stash", "pop", timeout=30).returncode == 0: + stashed = False + except subprocess.TimeoutExpired: + err += f"; the tree could not be reset to {pre_head[:12]} (reset timed out)" return err + (kept if stashed else "") @@ -987,13 +999,30 @@ def step() -> Optional[str]: ) if merge.returncode == 0: return None - run_registered(["git", "merge", "--abort"], cwd=str(home_dir), timeout=30) - return f"merging {source}: " + _summarize_git_error( - merge.stderr or merge.stdout or "merge failed") + # Git reports a conflict on STDOUT with an empty stderr, so the first + # output line is "Auto-merging " — read the unmerged paths + # BEFORE the abort clears them (mirrors `_rebase_onto_remote`). + conflicted = _unmerged_paths(home_dir) + output = f"{merge.stdout or ''}\n{merge.stderr or ''}" + abort = run_registered(["git", "merge", "--abort"], cwd=str(home_dir), timeout=30) + if conflicted or "CONFLICT" in output: + files = f" ({', '.join(conflicted[:3])})" if conflicted else "" + err = f"diverged: merge conflict with {source}{files}" + else: + err = f"merging {source}: " + _summarize_git_error( + merge.stderr or merge.stdout or "merge failed") + if abort.returncode != 0: + err += ("; merge --abort failed (" + + _summarize_git_error(abort.stderr or abort.stdout or "abort failed") + ")") + return err return _with_stash(home_dir, branch, step) +class _PullCountError(Exception): + """The pull cycle could not count a branch against origin.""" + + def _run_pull_once(home_dir: Path) -> Dict: """One pull cycle (trinity-enterprise#703, invariant G3): bring origin's commits to the agent. @@ -1004,9 +1033,11 @@ def _run_pull_once(home_dir: Path) -> Dict: the source branch (`_get_pull_branch`, normally `main`) is merged in, or human work pushed to `main` would never arrive (PR #3021 ruling). - - Never while an execution runs or is queued (a rebase under a turn changes - the files it is reading) — checked before the fetch and again right before - the tree is touched; an unreadable registry counts as busy. + - Never STARTS while an execution runs or is queued (a rebase under a turn + changes the files it is reading) — checked before the fetch and again + right before the tree is touched; an unreadable registry counts as busy. + Check-then-act: admission does not wait on a pull, so a turn admitted + during the integrate window can still see HEAD move. - Serialised with the push cycle and the operator git endpoints on `_REPO_LOCK`; a busy repo skips quietly (and unrecorded: every write to sync-state.json happens under that lock). @@ -1062,10 +1093,18 @@ def _run_pull_once(home_dir: Path) -> Dict: return _record_pull(home_dir, "failed", error=_summarize_git_error( retry.stderr or retry.stdout or "fetch failed")) - ahead, behind = _compute_ahead_behind(home_dir, branch) if own_on_origin else (0, 0) + # Strict counts (`_ahead_behind_vs`): the best-effort helper's (0, 0) on + # a failure would record an "up to date" success (PR #3021 re-review). + def count(ref: str) -> tuple: + counts = _ahead_behind_vs(home_dir, ref) + if counts is None: + raise _PullCountError(ref) + return counts + + ahead, behind = count(branch) if own_on_origin else (0, 0) source_behind = 0 if source != branch: - _, source_behind = _compute_ahead_behind(home_dir, source) + _, source_behind = count(source) if behind == 0 and source_behind == 0: return _record_pull(home_dir, "success", behind=0) @@ -1081,10 +1120,13 @@ def _run_pull_once(home_dir: Path) -> Dict: err = _integrate_source(home_dir, branch, source) if err: return _record_pull(home_dir, "failed", behind=0, error=err) - after = _compute_ahead_behind(home_dir, branch)[1] if own_on_origin else 0 + after = count(branch)[1] if own_on_origin else 0 logger.info("pull: %s brought in %s commit(s) on %s and %s from %s", home_dir, behind, branch, source_behind, source) return _record_pull(home_dir, "success", behind=after) + except _PullCountError as exc: + return _record_pull(home_dir, "failed", error=( + f"could not count commits on {exc} against origin")) except subprocess.CalledProcessError as exc: return _record_pull(home_dir, "failed", error=_summarize_git_error( exc.stderr or exc.stdout or str(exc))) diff --git a/docs/memory/architecture/agent-lifecycle.md b/docs/memory/architecture/agent-lifecycle.md index d82a4903d7..c87c886a7c 100644 --- a/docs/memory/architecture/agent-lifecycle.md +++ b/docs/memory/architecture/agent-lifecycle.md @@ -170,7 +170,7 @@ An agent's Claude Code marketplace-plugin selection is a first-class, **committe ### Git Sync Health (#389/#390) -**Pull cycle (trinity-enterprise#703):** a second loop beside the push loop, `auto_sync.run_pull_loop` → `routers/git.py::_run_pull_once`. It is gated per cycle on the owner's `pull_sync_enabled`, read live from `GET .../git/pull-sync`, with `GIT_SYNC_PULL` as the fallback; the interval is `GIT_SYNC_PULL_INTERVAL_SECONDS`, defaulting to the push interval. It never runs while an execution is in flight (process registry, checked under `_REPO_LOCK` and again before the tree is touched). `_integrate_remote` stashes uncommitted edits explicitly and undoes a pull whose incoming changes collide with them, instead of autostash, which would strand them in the stash. Outcome goes to `sync-state.json` (`last_pull_*`), persisted on `agent_sync_state`. +**Pull cycle (trinity-enterprise#703):** a second loop beside the push loop, `auto_sync.run_pull_loop` → `routers/git.py::_run_pull_once`. It is gated per cycle on the owner's `pull_sync_enabled`, read live from `GET .../git/pull-sync`, with `GIT_SYNC_PULL` as the fallback; the interval is `GIT_SYNC_PULL_INTERVAL_SECONDS`, defaulting to the push interval. It never starts while an execution is in flight or queued (process registry, checked under `_REPO_LOCK` and again before the tree is touched). That is check-then-act: turn admission (`register_pending`) does not wait on a pull, so a turn admitted during the integrate window can read files while HEAD moves. A working branch's merge of `main` survives the push cycle's rebase (`--rebase-merges`); a conflict with `main` is recorded as `diverged: merge conflict with main ()`. `_integrate_remote` stashes uncommitted edits explicitly and undoes a pull whose incoming changes collide with them, instead of autostash, which would strand them in the stash. Outcome goes to `sync-state.json` (`last_pull_*`), persisted on `agent_sync_state`. **Agent side:** 15-min `auto_sync` heartbeat loop in the agent server (each cycle gated live on the owner's `auto_sync_enabled`, read with the agent's own key from `GET .../git/auto-sync`, `GIT_SYNC_AUTO` as the unreachable-platform fallback — #3010; default-on for non-source-mode GitHub-template agents) stages/commits in-container changes, fetches and rebases onto `origin/` before a lease-protected push (a conflict aborts and records `diverged: …`; a source-mode agent on the default branch refuses — #3011), and writes the outcome to `.trinity/sync-state.json` (S1a; atomic tmp+`os.replace` write). **Maintenance ownership (#1595):** git's own auto-gc can NEVER complete in an agent container — it detaches to PID 1 and the #817 orphan sweep SIGKILLs it every time (44 GB / 97%-garbage repos observed) — so the base image disables it (`gc.auto=0`, `gc.autoDetach=false`, `maintenance.auto=false` in `/etc/gitconfig`; also in `git_service` setup for older images) and the auto-sync loop is the **single maintenance owner** for the agent's home repo (sub-repos cloned into the workspace get no maintenance — documented blind spot). The cycle runs in a worker thread (`asyncio.to_thread` — a long repack no longer starves `/health`/heartbeats), every subprocess is sweep-registered via `utils/registered_run.py` (`ProcessRegistry.add_transient_pid`, process-group timeout kill), and a non-blocking repo `threading.Lock` serializes it against the mutating git endpoints (`sync`/`pull`/reset → 409 `agent_busy` on contention; busy cycle skips). Each cycle reaps stale lock litter age-gated (`gc.pid`/`index.lock` >1h, `tmp_pack_*` > repack budget; `startup.sh` reaps unconditionally at container start — the stale-`index.lock` 12-day-freeze class). **#2742 — the status read stops racing the agent's own git:** `GET /api/git/status` (the surface the backend's 60 s poll drives) used to run ~8 bare `subprocess.run` git children on the event loop, including `git status --porcelain`, which takes `.git/index.lock` on EVERY invocation to refresh the index — so the platform took that lock ~2×/min in every workspace, outside `_REPO_LOCK`, and a sweep-killed child orphaned a 0-byte lock that silently failed every later git write. The read now uses `git --no-optional-locks status --porcelain -z` (scoped to this one call site — the commit paths still want the writeback), routes every child through `run_registered`, and is computed in ONE `to_thread` worker behind a loop-level single-flight so the poller, the UI git panel and the MCP tool coalesce onto one computation and one `git fetch origin` (`computed_at` stamps the resulting bounded staleness). A present `index.lock` is **reported, never deleted** — 0 bytes is the signature of a *live* `git add` for ~100 % of its life and `st_mtime` never advances, so staleness is decided by two-point inode stability (same inode unchanged across ≥3 ticks / ≥15 min), gitdir-resolved to cover worktrees and submodules, `lstat`-only (no repo lock, hence no new 409 `agent_busy`) and wrapped in its own `except OSError` so the observer can never darken the feed it feeds. Recovery stays where it is provably race-free — the boot reap, which now *announces* what it cleared and records it as `sync_state.last_lock_recovery`. **Bloat controls (#1596/#1595):** after a successful push the loop runs a consolidating **repack** (`git repack -A -d -l --unpack-unreachable=1.hour.ago` + `git gc --prune=1.hour.ago` — 1h grace so concurrent Claude-run git ops are never corrupted; `pack.threads=1` + `pack.windowMemory=128m` bound RSS) when packs ≥ `GIT_MAINTENANCE_PACK_THRESHOLD` (20) OR loose objects ≥ `GIT_MAINTENANCE_LOOSE_THRESHOLD` (6700 — with auto-gc off, garbage accumulates loose); guarded by a free-disk preflight (skip < 1.1× pack bytes), an exponential failure backoff (1h→24h, persisted in sync-state), and an env-tunable budget (`GIT_MAINTENANCE_TIMEOUT_SECONDS`, default 1800, read at call time). Non-destructive: does NOT rewrite reachable history. It measures `.git` size (`du -sb`) plus `git count-objects -v` pack/loose counts into sync-state on **every** terminal path (failing repos must not go dark). The fleet-wide default `.gitignore` (`git_service._GITIGNORE_PATTERNS`, **merged into each agent on sync and at creation** — #2069, readiness-gated) also excludes bulk data/deps/caches (`node_modules/`, `.venv/`, `__pycache__/`, `*.sqlite`/`*.db`, …) so churny non-source files aren't auto-committed. **Creation-time seed (#2069):** the list was applied only on Push/init, never at creation, so an agent whose 15-min auto-sync loop is on from birth (the `GIT_SYNC_AUTO` set — non-source/fork `github:` agents, ephemeral ghosts included) auto-committed `.trinity/` state + root `.env`/`.mcp.json` into its user-owned repo before any Push could migrate the list. `git_service.spawn_gitignore_merge_after_clone` fires a fire-and-forget, agent-`/health`-readiness-gated (server launches at `startup.sh:517` after ALL git setup, ~900s before the first cycle) merge — reusing `_build_gitignore_merge_command` (no fourth pattern list), merge-only (PREVENT: stops `git add -A` staging the untracked generated creds), gated on the shared `_git_auto_sync_baked` ENV predicate, idempotent (#953). Wired at creation (`_materialize_agent_files`) and at start/recreate (`start_agent_internal`, gated on the DB `auto_sync_enabled` flag — fleet remediation for existing leakers). **`.trinity/` is ignored contents-only (#2070):** `.trinity/*` plus a `!` re-include per path in `_TRINITY_AUTHORED_PATHS` (`pre-check`, `post-check`, `pre-snapshot`, `setup.sh`, `persistent-processes.allow`, `brain-orb/`, `pipelines/`, `plugins.yaml` #1704) — the files a TEMPLATE commits and the platform reads back. The wholesale `.trinity/` exclusion made every Push's `git rm --cached` untrack them and push the deletion, guarded only by a hardcoded two-entry pathspec that was wrong three times (brain-orb ent#76, `setup.sh`, then `pre-check` #2070); the symptom is silent because only the index is touched, surfacing one re-clone/recreate later as an agent with no hook. Now the default under `.trinity/` is *ignored*, so a new runtime file needs no action, while authored content is tracked by construction — and the `rm --cached` exemptions are DERIVED from the same constant so the two cannot drift. The merge also **removes** the superseded exact line `.trinity/` (git never descends into a dir-form exclusion, so the re-includes under it would be inert on every pre-#2070 agent). **Precedence — TWO managed regions (#2529):** the merge is a normalize-and-rebuild, not an append, because a canonical rule appended *below* an agent's negation silently reverses it and the sweep then untracks the file — and a canonical rule hoisted *above* a platform negation silently reverses that one instead, which is why there are two managed regions and not one. `[defaults block][user region, original order][protected floor]`: the agent's `!.env.example` / `!.claude/settings.json` beats a default without having to be the file's last line, while the seven credential patterns (`.env`, `.env.*`, `.mcp.json`, `credentials.json`, `*.pem`, `*.key`, `.ssh/` — membership decided against `credential_paths.py`, not against the constant's comment headings, which is how `.ssh/` was missed on the first pass) with their two canonical negations and `.trinity/*` with its eight derived `!` re-includes sit BELOW the user region and cannot be overridden (a single hoisted block would flip every currently-inert `!.env` in the fleet live in one unattended Push, and would let a user `*.sh` beat `!.trinity/setup.sh` — ent#76/#1704 reintroduced, and quietly, since the rm-cached pathspec still exempts the path). Idempotent by content: a second run computes byte-identical output and the file is only written when it differs. **Residual, reported not fixed:** 23 canonical patterns are dir-form, and git does not descend into an excluded directory, so a negation beneath one is inert at *any* position — contents-form conversion was costed and rejected (it would force git to stat `node_modules/`-sized trees on every `status`), so the Push instead reports it via `shadowed_negations`, alongside `removed_paths` and `unignored_paths` on the API response, the MCP result, the toast, the commit message (best-effort — `git rm --cached` only stages) and a durable `gitignore_untracked` operator-queue entry — all five gated on `GitignoreSweep.changed_tracking` (removed OR unignored), because the rebuild changes what is in the repo in BOTH directions and the addition is already in the remote's history. Maintenance bounds *garbage*, not *history* — unbounded history growth remains the deferred opt-in squash / geometric-repack follow-up. **Backend side** (details in [git-sync-health.md](../feature-flows/git-sync-health.md)): `SyncHealthService` polls git-enabled agents every 60s (#2742: from the `synchealth:leader` lease-holder only, so `--workers 2` runs one poll not two — **fail-open**, because an observability feed must not go dark when Redis is; the cadence is `SYNC_HEALTH_POLL_INTERVAL_SECONDS`, default unchanged at 60 s. One poller instead of two halves the rate `consecutive_failures` climbs, so `sync_failing` now fires at ~3 min rather than ~90 s), upserts `agent_sync_state` (`consecutive_failures` ++ on fail / reset on success; `ahead_working`/`behind_working` expose working-branch divergence, P6; `git_dir_bytes` #1596 + `pack_count`/`loose_objects`/`maintenance_failures` #1595 carry the repo-health curve — all agent-supplied ints coerced at the boundary, the `/health clone_status` convention), emits `sync_failing` operator-queue entries at ≥3 failures (S1) and edge-triggered `git_bloat` entries when `git_dir_bytes` crosses `GIT_DIR_ALERT_BYTES` (default 10 GiB) or maintenance fails 3× running (#1595 — the failure class was previously silent until the disk filled). Powers the dashboard sync dot, `GET /api/agents/sync-health` (now incl. `git_dir_bytes`), and `GET /api/fleet/sync-audit` — whose `duplicate_binding` flag marks agents sharing a `(github_repo, working_branch)` pair (§P5 silent-clobber setup) (S6, #390). diff --git a/docs/memory/feature-flows/git-sync-health.md b/docs/memory/feature-flows/git-sync-health.md index 64c82b1692..53f00edfcf 100644 --- a/docs/memory/feature-flows/git-sync-health.md +++ b/docs/memory/feature-flows/git-sync-health.md @@ -376,8 +376,14 @@ SessionStart hook, and the 2026-09-24 audit found agents up to 31 commits behind cycle does — a pull-only agent has no push cycle to do it. - Skipped while any execution is running **or queued** (`list_running()` + `list_pending_ids()`, #2433), checked again right before the tree is - touched; an unreadable registry counts as busy. + touched; an unreadable registry counts as busy. This gate is about + *starting*: turn admission does not wait on a pull, so a turn admitted + during the integrate window (up to 60 s for a merge, 120 s for a rebase) + can see HEAD move. - Refused while the index holds unmerged paths. + - Counts are strict (`_ahead_behind_vs`): a branch that can't be counted + against origin fails the pull (`could not count commits on …`) instead of + reading as up to date. - Fetches the **checked-out** branch: source-mode agents sit on their source branch, working-branch agents on theirs. - Nothing committed locally → a fast-forward. A local commit → a rebase, @@ -386,7 +392,11 @@ SessionStart hook, and the 2026-09-24 audit found agents up to 31 commits behind (`_get_pull_branch`, `main`) **merged** in (`_integrate_source`) — only the agent writes its working branch, so without this human pushes to `main` never arrive. A merge, not a rebase: the branch is already pushed. A - conflict is aborted and recorded as `merging main: …`. + conflict is aborted and recorded as `diverged: merge conflict with main + ()` (plus `merge --abort failed (…)` if the abort did). Every rebase + here and in the push cycle is `--rebase-merges`, so an unpushed merge of + `main` survives a later rebase onto the agent's own branch instead of being + flattened into copies of `main`'s commits. - **Uncommitted edits** (`_integrate_remote`) are stashed explicitly and re-applied. `--autostash` is deliberately not used: when the incoming commits touch the same files, its re-apply conflicts, leaves the edits *only in the @@ -394,7 +404,10 @@ SessionStart hook, and the 2026-09-24 audit found agents up to 31 commits behind back to the pre-pull HEAD, where the stash applies cleanly. The result is recorded as `local edits conflict with incoming changes on `. Every exit (`_with_stash`) — a git child timing out included — either puts - the edits back or says `local edits are kept in \`git stash\``; a failed + the edits back or says `local edits are kept in \`git stash\``. A timed-out + step is reset to the pre-pull HEAD whether or not anything was stashed (a + killed merge can leave a half-updated tree or a `MERGE_HEAD`); a reset that + can't run says `the tree could not be reset to (…)`. A failed `reset --hard` stops there and says so. The push cycle refuses to stage over unmerged paths, so conflict markers left by such a failure are never committed and pushed. diff --git a/docs/memory/requirements/github.md b/docs/memory/requirements/github.md index c9e7d3f811..e26cd700eb 100644 --- a/docs/memory/requirements/github.md +++ b/docs/memory/requirements/github.md @@ -882,8 +882,12 @@ - uncommitted edits stashed and re-applied explicitly - never discards local work: a conflict is aborted or undone, and recorded; an edit that could not be put back is named as kept in `git stash` - - never runs while an execution is in flight or queued, nor over unmerged - paths (and the push cycle never commits them) + - never STARTS while an execution is in flight or queued, nor over unmerged + paths (and the push cycle never commits them). The gate is check-then-act: + admission does not wait for a pull, so a turn accepted during the integrate + window (fast-forward or merge up to 60 s, rebase up to 120 s, plus the stash + steps) can read files while HEAD moves. Holding admission during a pull is a + follow-up. - **Flag:** per-agent `pull_sync_enabled` (both migration tracks), read live each cycle, with `GIT_SYNC_PULL` as the fallback; interval `GIT_SYNC_PULL_INTERVAL_SECONDS` (defaults to the push interval). diff --git a/src/frontend/src/components/GitSyncSettingsPanel.vue b/src/frontend/src/components/GitSyncSettingsPanel.vue index a0b43d61af..ce7f83632c 100644 --- a/src/frontend/src/components/GitSyncSettingsPanel.vue +++ b/src/frontend/src/components/GitSyncSettingsPanel.vue @@ -72,8 +72,8 @@ />

Brings in edits people and other agents push to the repository, and - merges main into a working branch. The pull never runs while - the agent is working or has a turn queued, and never discards its own + merges main into a working branch. A pull never starts while the + agent is working or has a turn queued, and never discards its own changes. With auto-sync on, each push also rebases onto GitHub first.

1 and cmd[1] == "merge" and "--abort" not in cmd: + real(cmd, *a, **kw) + if lock: + (Path(kw["cwd"]) / ".git" / "index.lock").write_text("") + raise subprocess.TimeoutExpired(cmd, kw.get("timeout", 1)) + return real(cmd, *a, **kw) + + monkeypatch.setattr(git_router, "run_registered", fake) + + def test_a_timed_out_step_on_a_clean_tree_is_reset(self, world, monkeypatch): + agent, _, human = world + before = _out(agent, "rev-parse", "HEAD") + _human_push(human, "role.md", "role v2\n") + self._merge_then_timeout(monkeypatch) + + result = git_router._run_pull_once(agent) + + assert result == {"status": "failed", "error": "git merge timed out"} + assert _out(agent, "rev-parse", "HEAD") == before + assert (agent / "role.md").read_text() == "role v1\n" + + def test_a_reset_blocked_by_the_killed_childs_lock_says_so(self, world, monkeypatch): + agent, _, human = world + _human_push(human, "role.md", "role v2\n") + self._merge_then_timeout(monkeypatch, lock=True) + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert result["error"].startswith("git merge timed out") + assert "could not be reset" in result["error"] + assert "index.lock" in result["error"] + + class TestWorkingBranchReceivesMain: """PR #3021 ruling: a `trinity/*` agent only ever pulled its own branch, so human work pushed to `main` never arrived (invariant G3).""" @@ -386,11 +448,63 @@ def test_a_conflicting_main_is_aborted_and_recorded(self, world): result = git_router._run_pull_once(agent) assert result["status"] == "failed" - assert result["error"].startswith("merging main: ") + # PR #3021 re-review: git prints the conflict on STDOUT with an empty + # stderr, so the first stdout line ("Auto-merging notes.md") used to be + # recorded — an error that reads like progress. + assert result["error"] == "diverged: merge conflict with main (notes.md)" + assert _state(agent)["last_pull_error"] == result["error"] assert _out(agent, "rev-parse", "HEAD") == before assert not (agent / ".git" / "MERGE_HEAD").exists() assert (agent / "notes.md").read_text() == "notes — the agent's\n" + def test_a_failed_merge_abort_is_named_in_the_error(self, world, monkeypatch): + agent, _, human = world + self._on_working_branch(agent) + _human_push(human, "notes.md", "notes — human\n") + real = git_router.run_registered + + def abort_fails(cmd, *a, **kw): + if cmd[1:3] == ["merge", "--abort"]: + return subprocess.CompletedProcess(cmd, 128, "", "fatal: could not reset index") + return real(cmd, *a, **kw) + + monkeypatch.setattr(git_router, "run_registered", abort_fails) + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert result["error"].startswith("diverged: merge conflict with main (notes.md)") + assert "merge --abort failed" in result["error"] + assert "could not reset index" in result["error"] + + def test_a_rebase_keeps_the_merge_the_pull_made(self, world): + """PR #3021 re-review: cycle 1 merges `main` into the working branch + (not pushed yet), then someone pushes to the agent's own branch. A plain + `git rebase` in cycle 2 flattened the merge — `main`'s commit came back + as a copy and `origin/main` stopped being an ancestor, so `behind_main` + kept reporting the agent as behind.""" + agent, origin, human = world + self._on_working_branch(agent) + main_sha = _human_push(human, "role.md", "role v2 — from main\n") + assert git_router._run_pull_once(agent)["status"] == "success" # the merge + assert _out(agent, "log", "--merges", "--format=%H") + + _git(human, "fetch", "-q", "origin") + _git(human, "checkout", "-q", "-b", "wb", "origin/trinity/agent/1") + (human / "extra.md").write_text("pushed to the agent's branch\n") + _git(human, "add", "extra.md") + _git(human, "commit", "-q", "-m", "human: extra on the working branch") + _git(human, "push", "-q", "origin", "HEAD:trinity/agent/1") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "success" + assert (agent / "extra.md").exists() + assert _out(agent, "log", "--merges", "--format=%H") + assert _git(agent, "merge-base", "--is-ancestor", main_sha, "HEAD", + check=False).returncode == 0 + subjects = _out(agent, "log", "--format=%s").splitlines() + assert subjects.count("human: role.md") == 1 + class TestLoopGate: @pytest.fixture(autouse=True) From f1bee1340eb2c9206017385b8504a0dda064ed39 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 30 Sep 2026 10:56:19 +0300 Subject: [PATCH 6/8] fix(git-sync): pull-sync write is person-only; classify the agent's read #2996's route census (merged to dev) requires every new route to be classified. The agent's pull loop reads GET .../git/pull-sync with its own key each cycle (AGENT_CALLABLE); the PUT is a setting write, so it takes Depends(require_person) like the other #2996 settings. Co-Authored-By: Claude Opus 5.5 --- src/backend/routers/git.py | 2 ++ tests/unit/_route_census.py | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/src/backend/routers/git.py b/src/backend/routers/git.py index a5a4ada39a..fa7c4740cd 100644 --- a/src/backend/routers/git.py +++ b/src/backend/routers/git.py @@ -32,6 +32,7 @@ from dependencies import ( get_current_user, reject_agent_principal, + require_person, AuthorizedAgentByName, OwnedAgentByName, ) @@ -1203,6 +1204,7 @@ async def get_pull_sync_config(agent_name: AuthorizedAgentByName): async def set_pull_sync_config( agent_name: OwnedAgentByName, body: PullSyncToggle, + current_user: User = Depends(require_person), ): """trinity-enterprise#703: turn the container's pull cycle on or off; live on the agent's next pull cycle, no recreate.""" diff --git a/tests/unit/_route_census.py b/tests/unit/_route_census.py index 2be6a3c897..ae9da36c90 100644 --- a/tests/unit/_route_census.py +++ b/tests/unit/_route_census.py @@ -556,6 +556,10 @@ def _resolve_endpoint( # only the agent's own key (or trinity-system as itself) and refuses every person. "routers/operator_queue.py::raise_my_ask": ( "POST /api/agents/{name}/operator-queue", "MCP ask_operator; the agent raises its own ask, self-only (get_self_acting_agent)"), + # ent#703: the agent's pull loop reads its own switch every cycle with its own key. + # The write (PUT .../git/pull-sync) is a setting, so it is person-only. + "routers/git.py::get_pull_sync_config": ( + "GET /api/agents/{agent_name}/git/pull-sync", "ent#703 the agent's pull loop reads its own switch live each cycle"), } # Authenticates itself, or unauthenticated by design. key -> ("METHOD /full/path", reason) From e6cb8b5b79022f061413a50518440f97149d7e7a Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Fri, 2 Oct 2026 17:26:20 +0300 Subject: [PATCH 7/8] fix(git-sync): the push and pull cycles share the repo lock without starving each other (#3021 review) The two background loops started together on one interval and took _REPO_LOCK non-blocking, so the loser skipped silently every tick and the effective pull bound could quietly double. Background cycles now wait up to GIT_SYNC_LOCK_WAIT_SECONDS (default 120 s) for the lock, and the pull loop's first tick is half an interval after the push loop's. Operator endpoints keep their immediate 409. The pull's reset --hard undo re-checks that no execution started meanwhile; if one did, the tree is left as it is (conflict markers block the next push and pull) instead of discarding the turn's writes. merge --abort runs only when MERGE_HEAD exists, so a merge that refused to start no longer records "There is no merge to abort". Co-Authored-By: Claude Opus 5.5 --- docker/base-image/agent_server/auto_sync.py | 5 +- docker/base-image/agent_server/routers/git.py | 77 +++++++++-- docs/memory/architecture/agent-lifecycle.md | 2 +- docs/memory/feature-flows/git-sync-health.md | 8 +- docs/memory/requirements/github.md | 10 +- tests/unit/test_1595_git_maintenance.py | 5 +- tests/unit/test_ent703_pull_cycle.py | 120 +++++++++++++++++- 7 files changed, 213 insertions(+), 14 deletions(-) diff --git a/docker/base-image/agent_server/auto_sync.py b/docker/base-image/agent_server/auto_sync.py index c6578445be..1ad19f149f 100644 --- a/docker/base-image/agent_server/auto_sync.py +++ b/docker/base-image/agent_server/auto_sync.py @@ -248,7 +248,10 @@ async def run_pull_loop( await resolve_pull_sync_enabled(client) except Exception: # noqa: BLE001 — loop must never die logger.exception("pull: initial flag read raised unexpectedly") - await asyncio.sleep(interval) + # Half an interval later than the push loop (PR #3021 review): the two + # start together and, on the default shared interval, would otherwise + # reach the repo lock on the same tick every time. + await asyncio.sleep(interval + interval / 2) while True: try: await run_one_pull_cycle(client, home, _run_pull_once) diff --git a/docker/base-image/agent_server/routers/git.py b/docker/base-image/agent_server/routers/git.py index 3945639ee1..442f78da82 100644 --- a/docker/base-image/agent_server/routers/git.py +++ b/docker/base-image/agent_server/routers/git.py @@ -556,6 +556,52 @@ def _index_lock_stuck(home_dir: Path) -> Optional[Dict]: # server's _REPO_LOCK"). Never treat holding it as "no git is running here". _REPO_LOCK = threading.Lock() +# PR #3021 review: the push and pull cycles are both background loops on the +# same lock. Taking it non-blocking made the loser skip silently — and two +# loops on one interval collide every tick, quietly halving the pull bound. +# The background cycles wait this long for the other one to finish; operator +# endpoints keep their immediate 409 (a person is waiting on those). +_DEFAULT_CYCLE_LOCK_WAIT_SECONDS = 120 + + +def _cycle_lock_wait_seconds() -> float: + """`GIT_SYNC_LOCK_WAIT_SECONDS` (default 120; 0 = do not wait).""" + raw = os.getenv("GIT_SYNC_LOCK_WAIT_SECONDS") + try: + value = float(raw) if raw else _DEFAULT_CYCLE_LOCK_WAIT_SECONDS + except ValueError: + return _DEFAULT_CYCLE_LOCK_WAIT_SECONDS + return max(0.0, value) + + +def _acquire_for_cycle() -> bool: + """The repo lock for a BACKGROUND cycle: wait a bounded time for the other + cycle (or an operator op) to finish, then give up as `repo_busy`.""" + wait = _cycle_lock_wait_seconds() + if wait <= 0: + return _REPO_LOCK.acquire(blocking=False) + return _REPO_LOCK.acquire(timeout=wait) + + +def _merge_in_progress(home_dir: Path) -> bool: + """Is a merge actually under way (MERGE_HEAD exists)? `merge --abort` + without one fails with "There is no merge to abort" — noise in the + recorded error, not information (PR #3021 review).""" + probe = run_registered(["git", "rev-parse", "-q", "--verify", "MERGE_HEAD"], + cwd=str(home_dir), timeout=10) + return probe.returncode == 0 + + +def _safe_to_reset() -> bool: + """May the pull reset the tree back to its pre-pull HEAD right now? + + `reset --hard` discards every tracked-file write since the stash — so a + turn admitted after the pull's own in-flight check (admission does not wait + on a pull) would lose what it wrote. Re-checked immediately before each + reset; an unreadable registry counts as busy (PR #3021 review).""" + running = _executions_in_flight() + return running == 0 + def _with_repo_lock(fn): """Guard a mutating git endpoint with the repo lock (409 on contention).""" @@ -1089,7 +1135,14 @@ def git(*args, timeout=60): stashed = False return None # The edits collide with what came in. Undo the pull — the stash is - # still there — and re-apply on the base it was taken from. + # still there — and re-apply on the base it was taken from. Unless an + # execution started meanwhile: then a reset would discard what it + # wrote, so the tree is left as it is (its conflict markers make the + # push cycle and the next pull refuse until a person resolves them). + if not _safe_to_reset(): + return (f"local edits conflict with incoming changes on {branch}; an " + f"execution started meanwhile, so the tree was left as it is — " + f"resolve the conflicts" + kept) reset = git("reset", "--hard", pre_head, timeout=30) if reset.returncode != 0: return (f"local edits conflict with incoming changes on {branch} and the " @@ -1107,7 +1160,11 @@ def git(*args, timeout=60): # next push cycle would commit as the agent's own work (PR #3021 # re-review). Then re-apply the stash on the base it was taken from. try: - git("merge", "--abort", timeout=30) # no-op unless MERGE_HEAD exists + if _merge_in_progress(home_dir): + git("merge", "--abort", timeout=30) + if not _safe_to_reset(): + return (err + f"; an execution started meanwhile, so the tree was not " + f"reset to {pre_head[:12]}" + (kept if stashed else "")) reset = git("reset", "--hard", pre_head, timeout=30) if reset.returncode != 0: err += (f"; the tree could not be reset to {pre_head[:12]} (" @@ -1154,14 +1211,18 @@ def step() -> Optional[str]: # BEFORE the abort clears them (mirrors `_rebase_onto_remote`). conflicted = _unmerged_paths(home_dir) output = f"{merge.stdout or ''}\n{merge.stderr or ''}" - abort = run_registered(["git", "merge", "--abort"], cwd=str(home_dir), timeout=30) + # A merge that refused to START (an untracked file it would overwrite) + # leaves no MERGE_HEAD; aborting it only appends git's "There is no + # merge to abort" to the recorded error. + abort = (run_registered(["git", "merge", "--abort"], cwd=str(home_dir), timeout=30) + if _merge_in_progress(home_dir) else None) if conflicted or "CONFLICT" in output: files = f" ({', '.join(conflicted[:3])})" if conflicted else "" err = f"diverged: merge conflict with {source}{files}" else: err = f"merging {source}: " + _summarize_git_error( merge.stderr or merge.stdout or "merge failed") - if abort.returncode != 0: + if abort is not None and abort.returncode != 0: err += ("; merge --abort failed (" + _summarize_git_error(abort.stderr or abort.stdout or "abort failed") + ")") return err @@ -1200,7 +1261,7 @@ def _run_pull_once(home_dir: Path) -> Dict: `origin/`, not its lag behind `main`; `behind_main` in the git status is the latter. """ - if not _REPO_LOCK.acquire(blocking=False): + if not _acquire_for_cycle(): return {"status": "skipped", "reason": "repo_busy"} try: _reap_stale_git_litter( @@ -1305,9 +1366,9 @@ def _run_auto_sync_once(home_dir: Path) -> Dict: """ now = datetime.now(timezone.utc).isoformat() - if not _REPO_LOCK.acquire(blocking=False): - # Operator op in flight — skip quietly; not a sync failure. - logger.info("auto-sync: repo busy (operator git op in flight), skipping cycle") + if not _acquire_for_cycle(): + # Still held after the bounded wait — skip quietly; not a sync failure. + logger.info("auto-sync: repo busy (git op still in flight after the wait), skipping cycle") return {"status": "skipped", "reason": "repo_busy"} try: diff --git a/docs/memory/architecture/agent-lifecycle.md b/docs/memory/architecture/agent-lifecycle.md index 087d96ae02..f334916cf9 100644 --- a/docs/memory/architecture/agent-lifecycle.md +++ b/docs/memory/architecture/agent-lifecycle.md @@ -170,7 +170,7 @@ An agent's Claude Code marketplace-plugin selection is a first-class, **committe ### Git Sync Health (#389/#390) -**Pull cycle (trinity-enterprise#703):** a second loop beside the push loop, `auto_sync.run_pull_loop` → `routers/git.py::_run_pull_once`. It is gated per cycle on the owner's `pull_sync_enabled`, read live from `GET .../git/pull-sync`, with `GIT_SYNC_PULL` as the fallback; the interval is `GIT_SYNC_PULL_INTERVAL_SECONDS`, defaulting to the push interval. It never starts while an execution is in flight or queued (process registry, checked under `_REPO_LOCK` and again before the tree is touched). That is check-then-act: turn admission (`register_pending`) does not wait on a pull, so a turn admitted during the integrate window can read files while HEAD moves. A working branch's merge of `main` survives the push cycle's rebase (`--rebase-merges`); a conflict with `main` is recorded as `diverged: merge conflict with main ()`. `_integrate_remote` stashes uncommitted edits explicitly and undoes a pull whose incoming changes collide with them, instead of autostash, which would strand them in the stash. Outcome goes to `sync-state.json` (`last_pull_*`), persisted on `agent_sync_state`. +**Pull cycle (trinity-enterprise#703):** a second loop beside the push loop, `auto_sync.run_pull_loop` → `routers/git.py::_run_pull_once`. It is gated per cycle on the owner's `pull_sync_enabled`, read live from `GET .../git/pull-sync`, with `GIT_SYNC_PULL` as the fallback; the interval is `GIT_SYNC_PULL_INTERVAL_SECONDS`, defaulting to the push interval, with the first tick half an interval after the push loop's. Both background cycles wait up to `GIT_SYNC_LOCK_WAIT_SECONDS` (default 120 s) for `_REPO_LOCK` instead of skipping, and a pull's `reset --hard` undo re-checks that no execution started first (`_safe_to_reset`). It never starts while an execution is in flight or queued (process registry, checked under `_REPO_LOCK` and again before the tree is touched). That is check-then-act: turn admission (`register_pending`) does not wait on a pull, so a turn admitted during the integrate window can read files while HEAD moves. A working branch's merge of `main` survives the push cycle's rebase (`--rebase-merges`); a conflict with `main` is recorded as `diverged: merge conflict with main ()`. `_integrate_remote` stashes uncommitted edits explicitly and undoes a pull whose incoming changes collide with them, instead of autostash, which would strand them in the stash. Outcome goes to `sync-state.json` (`last_pull_*`), persisted on `agent_sync_state`. **Agent side:** 15-min `auto_sync` heartbeat loop in the agent server (each cycle gated live on the owner's `auto_sync_enabled`, read with the agent's own key from `GET .../git/auto-sync`, `GIT_SYNC_AUTO` as the unreachable-platform fallback — #3010; default-on for non-source-mode GitHub-template agents) stages/commits in-container changes (the platform heartbeat is the durability owner on a deployed agent; a `.claude/settings.json` carrying container paths or credential-bearing keys is kept out of every commit by a content guard rather than an ignore — ent#708), fetches and rebases onto `origin/` before a lease-protected push (a conflict aborts and records `diverged: …`; a source-mode agent on the default branch refuses — #3011), and writes the outcome to `.trinity/sync-state.json` (S1a; atomic tmp+`os.replace` write). **Maintenance ownership (#1595):** git's own auto-gc can NEVER complete in an agent container — it detaches to PID 1 and the #817 orphan sweep SIGKILLs it every time (44 GB / 97%-garbage repos observed) — so the base image disables it (`gc.auto=0`, `gc.autoDetach=false`, `maintenance.auto=false` in `/etc/gitconfig`; also in `git_service` setup for older images) and the auto-sync loop is the **single maintenance owner** for the agent's home repo (sub-repos cloned into the workspace get no maintenance — documented blind spot). The cycle runs in a worker thread (`asyncio.to_thread` — a long repack no longer starves `/health`/heartbeats), every subprocess is sweep-registered via `utils/registered_run.py` (`ProcessRegistry.add_transient_pid`, process-group timeout kill), and a non-blocking repo `threading.Lock` serializes it against the mutating git endpoints (`sync`/`pull`/reset → 409 `agent_busy` on contention; busy cycle skips). Each cycle reaps stale lock litter age-gated (`gc.pid`/`index.lock` >1h, `tmp_pack_*` > repack budget; `startup.sh` reaps unconditionally at container start — the stale-`index.lock` 12-day-freeze class). **#2742 — the status read stops racing the agent's own git:** `GET /api/git/status` (the surface the backend's 60 s poll drives) used to run ~8 bare `subprocess.run` git children on the event loop, including `git status --porcelain`, which takes `.git/index.lock` on EVERY invocation to refresh the index — so the platform took that lock ~2×/min in every workspace, outside `_REPO_LOCK`, and a sweep-killed child orphaned a 0-byte lock that silently failed every later git write. The read now uses `git --no-optional-locks status --porcelain -z` (scoped to this one call site — the commit paths still want the writeback), routes every child through `run_registered`, and is computed in ONE `to_thread` worker behind a loop-level single-flight so the poller, the UI git panel and the MCP tool coalesce onto one computation and one `git fetch origin` (`computed_at` stamps the resulting bounded staleness). A present `index.lock` is **reported, never deleted** — 0 bytes is the signature of a *live* `git add` for ~100 % of its life and `st_mtime` never advances, so staleness is decided by two-point inode stability (same inode unchanged across ≥3 ticks / ≥15 min), gitdir-resolved to cover worktrees and submodules, `lstat`-only (no repo lock, hence no new 409 `agent_busy`) and wrapped in its own `except OSError` so the observer can never darken the feed it feeds. Recovery stays where it is provably race-free — the boot reap, which now *announces* what it cleared and records it as `sync_state.last_lock_recovery`. **Bloat controls (#1596/#1595):** after a successful push the loop runs a consolidating **repack** (`git repack -A -d -l --unpack-unreachable=1.hour.ago` + `git gc --prune=1.hour.ago` — 1h grace so concurrent Claude-run git ops are never corrupted; `pack.threads=1` + `pack.windowMemory=128m` bound RSS) when packs ≥ `GIT_MAINTENANCE_PACK_THRESHOLD` (20) OR loose objects ≥ `GIT_MAINTENANCE_LOOSE_THRESHOLD` (6700 — with auto-gc off, garbage accumulates loose); guarded by a free-disk preflight (skip < 1.1× pack bytes), an exponential failure backoff (1h→24h, persisted in sync-state), and an env-tunable budget (`GIT_MAINTENANCE_TIMEOUT_SECONDS`, default 1800, read at call time). Non-destructive: does NOT rewrite reachable history. It measures `.git` size (`du -sb`) plus `git count-objects -v` pack/loose counts into sync-state on **every** terminal path (failing repos must not go dark). The fleet-wide default `.gitignore` (`git_service._GITIGNORE_PATTERNS`, **merged into each agent on sync and at creation** — #2069, readiness-gated) also excludes bulk data/deps/caches (`node_modules/`, `.venv/`, `__pycache__/`, `*.sqlite`/`*.db`, …) so churny non-source files aren't auto-committed. **Creation-time seed (#2069):** the list was applied only on Push/init, never at creation, so an agent whose 15-min auto-sync loop is on from birth (the `GIT_SYNC_AUTO` set — non-source/fork `github:` agents, ephemeral ghosts included) auto-committed `.trinity/` state + root `.env`/`.mcp.json` into its user-owned repo before any Push could migrate the list. `git_service.spawn_gitignore_merge_after_clone` fires a fire-and-forget, agent-`/health`-readiness-gated (server launches at `startup.sh:517` after ALL git setup, ~900s before the first cycle) merge — reusing `_build_gitignore_merge_command` (no fourth pattern list), merge-only (PREVENT: stops `git add -A` staging the untracked generated creds), gated on the shared `_git_auto_sync_baked` ENV predicate, idempotent (#953). Wired at creation (`_materialize_agent_files`) and at start/recreate (`start_agent_internal`, gated on the DB `auto_sync_enabled` flag — fleet remediation for existing leakers). **`.trinity/` is ignored contents-only (#2070):** `.trinity/*` plus a `!` re-include per path in `_TRINITY_AUTHORED_PATHS` (`pre-check`, `post-check`, `pre-snapshot`, `setup.sh`, `persistent-processes.allow`, `brain-orb/`, `pipelines/`, `plugins.yaml` #1704) — the files a TEMPLATE commits and the platform reads back. The wholesale `.trinity/` exclusion made every Push's `git rm --cached` untrack them and push the deletion, guarded only by a hardcoded two-entry pathspec that was wrong three times (brain-orb ent#76, `setup.sh`, then `pre-check` #2070); the symptom is silent because only the index is touched, surfacing one re-clone/recreate later as an agent with no hook. Now the default under `.trinity/` is *ignored*, so a new runtime file needs no action, while authored content is tracked by construction — and the `rm --cached` exemptions are DERIVED from the same constant so the two cannot drift. The merge also **removes** the superseded exact line `.trinity/` (git never descends into a dir-form exclusion, so the re-includes under it would be inert on every pre-#2070 agent). **Precedence — TWO managed regions (#2529):** the merge is a normalize-and-rebuild, not an append, because a canonical rule appended *below* an agent's negation silently reverses it and the sweep then untracks the file — and a canonical rule hoisted *above* a platform negation silently reverses that one instead, which is why there are two managed regions and not one. `[defaults block][user region, original order][protected floor]`: the agent's `!.env.example` / `!.claude/settings.json` beats a default without having to be the file's last line, while the seven credential patterns (`.env`, `.env.*`, `.mcp.json`, `credentials.json`, `*.pem`, `*.key`, `.ssh/` — membership decided against `credential_paths.py`, not against the constant's comment headings, which is how `.ssh/` was missed on the first pass) with their two canonical negations and `.trinity/*` with its eight derived `!` re-includes sit BELOW the user region and cannot be overridden (a single hoisted block would flip every currently-inert `!.env` in the fleet live in one unattended Push, and would let a user `*.sh` beat `!.trinity/setup.sh` — ent#76/#1704 reintroduced, and quietly, since the rm-cached pathspec still exempts the path). Idempotent by content: a second run computes byte-identical output and the file is only written when it differs. **Residual, reported not fixed:** 23 canonical patterns are dir-form, and git does not descend into an excluded directory, so a negation beneath one is inert at *any* position — contents-form conversion was costed and rejected (it would force git to stat `node_modules/`-sized trees on every `status`), so the Push instead reports it via `shadowed_negations`, alongside `removed_paths` and `unignored_paths` on the API response, the MCP result, the toast, the commit message (best-effort — `git rm --cached` only stages) and a durable `gitignore_untracked` operator-queue entry — all five gated on `GitignoreSweep.changed_tracking` (removed OR unignored), because the rebuild changes what is in the repo in BOTH directions and the addition is already in the remote's history. Maintenance bounds *garbage*, not *history* — unbounded history growth remains the deferred opt-in squash / geometric-repack follow-up. **Backend side** (details in [git-sync-health.md](../feature-flows/git-sync-health.md)): `SyncHealthService` polls git-enabled agents every 60s (#2742: from the `synchealth:leader` lease-holder only, so `--workers 2` runs one poll not two — **fail-open**, because an observability feed must not go dark when Redis is; the cadence is `SYNC_HEALTH_POLL_INTERVAL_SECONDS`, default unchanged at 60 s. One poller instead of two halves the rate `consecutive_failures` climbs, so `sync_failing` now fires at ~3 min rather than ~90 s), upserts `agent_sync_state` (`consecutive_failures` ++ on fail / reset on success; `ahead_working`/`behind_working` expose working-branch divergence, P6; `git_dir_bytes` #1596 + `pack_count`/`loose_objects`/`maintenance_failures` #1595 carry the repo-health curve — all agent-supplied ints coerced at the boundary, the `/health clone_status` convention), emits `sync_failing` operator-queue entries at ≥3 failures (S1) and edge-triggered `git_bloat` entries when `git_dir_bytes` crosses `GIT_DIR_ALERT_BYTES` (default 10 GiB) or maintenance fails 3× running (#1595 — the failure class was previously silent until the disk filled). Powers the dashboard sync dot, `GET /api/agents/sync-health` (now incl. `git_dir_bytes`), and `GET /api/fleet/sync-audit` — whose `duplicate_binding` flag marks agents sharing a `(github_repo, working_branch)` pair (§P5 silent-clobber setup) (S6, #390). diff --git a/docs/memory/feature-flows/git-sync-health.md b/docs/memory/feature-flows/git-sync-health.md index 147c06f906..b5ecd83f27 100644 --- a/docs/memory/feature-flows/git-sync-health.md +++ b/docs/memory/feature-flows/git-sync-health.md @@ -458,7 +458,13 @@ operator's, the fleet orchestrator's daily sweep, or a marketplace SessionStart hook, and the 2026-09-24 audit found agents up to 31 commits behind. - **Loop:** `auto_sync.run_pull_loop`, beside the push loop, every - `GIT_SYNC_PULL_INTERVAL_SECONDS` (defaults to the push interval, 900 s). + `GIT_SYNC_PULL_INTERVAL_SECONDS` (defaults to the push interval, 900 s). Its + first tick is half an interval after the push loop's, so the two do not reach + the repo lock together. +- **Lock:** both background cycles wait up to `GIT_SYNC_LOCK_WAIT_SECONDS` + (default 120 s; `0` = skip at once) for `_REPO_LOCK` instead of skipping on + sight. Skipping made two loops on one interval collide every tick and halve + the pull bound silently. Operator endpoints keep their immediate 409. - **Gate:** each cycle reads the owner's `pull_sync_enabled` live (`GET .../git/pull-sync`, agent key), with `GIT_SYNC_PULL` as the fallback, the #3010 pattern. diff --git a/docs/memory/requirements/github.md b/docs/memory/requirements/github.md index cb1e81810d..d6fe4a7bfc 100644 --- a/docs/memory/requirements/github.md +++ b/docs/memory/requirements/github.md @@ -894,7 +894,15 @@ follow-up. - **Flag:** per-agent `pull_sync_enabled` (both migration tracks), read live each cycle, with `GIT_SYNC_PULL` as the fallback; interval - `GIT_SYNC_PULL_INTERVAL_SECONDS` (defaults to the push interval). + `GIT_SYNC_PULL_INTERVAL_SECONDS` (defaults to the push interval), first tick + offset half an interval from the push loop. The push and pull cycles wait up + to `GIT_SYNC_LOCK_WAIT_SECONDS` (default 120 s) for the shared repo lock rather + than skipping, so neither starves the other. +- **Undo is never a reset over a turn:** the pull undoes a collision with + uncommitted edits by `reset --hard` to its pre-pull HEAD only after + re-checking that no execution started meanwhile; otherwise the tree is left + as it is (conflict markers block the next push and pull) and the failure + says so. `merge --abort` runs only when a merge is in progress. - on for new `github:` agents, source-mode included - on for existing agents only where auto-sync is already on - off otherwise; toggle in Settings → Git sync diff --git a/tests/unit/test_1595_git_maintenance.py b/tests/unit/test_1595_git_maintenance.py index 085cff5f65..88a549cffc 100644 --- a/tests/unit/test_1595_git_maintenance.py +++ b/tests/unit/test_1595_git_maintenance.py @@ -433,7 +433,10 @@ def test_write_is_atomic_no_tmp_left_behind(self, repo): class TestAutoSyncCycle: - def test_skips_when_repo_lock_held(self, repo): + def test_skips_when_repo_lock_held(self, repo, monkeypatch): + # PR #3021: a background cycle now waits a bounded time for the lock; + # 0 restores the immediate skip this test pins. + monkeypatch.setenv("GIT_SYNC_LOCK_WAIT_SECONDS", "0") assert _REPO_LOCK.acquire(blocking=False) try: result = _run_auto_sync_once(repo) diff --git a/tests/unit/test_ent703_pull_cycle.py b/tests/unit/test_ent703_pull_cycle.py index 7eee19a163..4cadfa3017 100644 --- a/tests/unit/test_ent703_pull_cycle.py +++ b/tests/unit/test_ent703_pull_cycle.py @@ -178,8 +178,9 @@ def test_an_execution_starting_mid_cycle_stops_it_before_the_tree(self, world, m assert result["status"] == "skipped" assert _out(agent, "rev-parse", "HEAD") == before - def test_a_busy_repo_is_skipped_without_writing_state(self, world): + def test_a_busy_repo_is_skipped_without_writing_state(self, world, monkeypatch): agent, _, _ = world + monkeypatch.setenv("GIT_SYNC_LOCK_WAIT_SECONDS", "0") # the immediate-skip path assert git_router._REPO_LOCK.acquire(blocking=False) try: assert git_router._run_pull_once(agent) == {"status": "skipped", "reason": "repo_busy"} @@ -558,6 +559,123 @@ def test_pull_interval_defaults_to_the_push_interval(self, monkeypatch): assert auto_sync.get_pull_interval_seconds() == 300 +# --------------------------------------------------------------------------- +# PR #3021 third review: the two loops share the lock without starving each +# other; a reset never runs over a turn's writes; no spurious merge --abort. +# --------------------------------------------------------------------------- + +class TestThirdReviewFixes: + def test_a_background_cycle_waits_for_the_lock_instead_of_skipping(self, world, monkeypatch): + """The push and pull loops share `_REPO_LOCK`. Skipping on a busy lock + made two loops on one interval collide every tick and silently halve + the pull bound; a cycle now waits for the other to finish.""" + import threading + import time + agent, _, human = world + _human_push(human, "role.md", "role v2\n") + monkeypatch.setenv("GIT_SYNC_LOCK_WAIT_SECONDS", "10") + assert git_router._REPO_LOCK.acquire(blocking=False) + threading.Timer(0.3, git_router._REPO_LOCK.release).start() + + started = time.monotonic() + result = git_router._run_pull_once(agent) + + assert time.monotonic() - started >= 0.25 + assert result["status"] == "success" + assert (agent / "role.md").read_text() == "role v2\n" + + def test_the_wait_is_bounded(self, world, monkeypatch): + import time + agent, _, _ = world + monkeypatch.setenv("GIT_SYNC_LOCK_WAIT_SECONDS", "0.3") + assert git_router._REPO_LOCK.acquire(blocking=False) + try: + started = time.monotonic() + assert git_router._run_pull_once(agent) == {"status": "skipped", "reason": "repo_busy"} + assert time.monotonic() - started < 5 + finally: + git_router._REPO_LOCK.release() + + @pytest.mark.parametrize("raw, expected", [(None, 120.0), ("0", 0.0), ("-5", 0.0), ("bad", 120.0), ("2.5", 2.5)]) + def test_the_wait_setting(self, monkeypatch, raw, expected): + if raw is None: + monkeypatch.delenv("GIT_SYNC_LOCK_WAIT_SECONDS", raising=False) + else: + monkeypatch.setenv("GIT_SYNC_LOCK_WAIT_SECONDS", raw) + assert git_router._cycle_lock_wait_seconds() == expected + + def test_the_pull_loop_starts_half_an_interval_after_the_push_loop(self, tmp_path, monkeypatch): + for var in ("TRINITY_BACKEND_URL", "TRINITY_MCP_API_KEY"): + monkeypatch.delenv(var, raising=False) + slept = [] + + async def first_sleep(seconds): + slept.append(seconds) + raise asyncio.CancelledError + + monkeypatch.setattr(auto_sync.asyncio, "sleep", first_sleep) + with pytest.raises(asyncio.CancelledError): + asyncio.new_event_loop().run_until_complete( + auto_sync.run_pull_loop(tmp_path, interval_seconds=100)) + assert slept == [150] + + def test_no_reset_over_a_turn_that_started_mid_pull(self, world, monkeypatch): + """Incoming changes collide with uncommitted edits, so the pull would + normally be undone with `reset --hard`. A turn admitted after the + pull's in-flight checks wrote a file meanwhile: the reset would discard + it, so the tree is left as it is and the failure says so.""" + agent, _, human = world + _human_push(human, "notes.md", "notes — human rewrite\n") + (agent / "notes.md").write_text("notes — agent's uncommitted edit\n") + calls = {"n": 0} + + def in_flight(): + calls["n"] += 1 + if calls["n"] >= 3: # the pre-reset check: a turn has started and written + (agent / "role.md").write_text("role — written by the turn\n") + return 1 + return 0 + + monkeypatch.setattr(git_router, "_executions_in_flight", in_flight) + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert "an execution started meanwhile, so the tree was left as it is" in result["error"] + assert "local edits are kept in `git stash`" in result["error"] + assert (agent / "role.md").read_text() == "role — written by the turn\n" + assert "trinity-pull" in _out(agent, "stash", "list") + + def test_a_reset_still_undoes_the_pull_when_nothing_runs(self, world): + """The unchanged path, pinned beside the new one: idle → undone.""" + agent, _, human = world + before = _out(agent, "rev-parse", "HEAD") + _human_push(human, "notes.md", "notes — human rewrite\n") + (agent / "notes.md").write_text("notes — agent's uncommitted edit\n") + + result = git_router._run_pull_once(agent) + + assert result["error"] == "local edits conflict with incoming changes on main" + assert _out(agent, "rev-parse", "HEAD") == before + + def test_a_merge_that_never_started_is_not_aborted(self, world): + """`git merge` refuses up front when an untracked file would be + overwritten: no MERGE_HEAD, so no `merge --abort` and none of git's + "There is no merge to abort" in the recorded error.""" + agent, _, human = world + TestWorkingBranchReceivesMain._on_working_branch(agent) + _human_push(human, "brief.md", "brief — from main\n") + (agent / "brief.md").write_text("brief — the agent's, untracked\n") + + result = git_router._run_pull_once(agent) + + assert result["status"] == "failed" + assert result["error"].startswith("merging main: ") + assert "merge --abort" not in result["error"] + assert "no merge to abort" not in result["error"].lower() + assert (agent / "brief.md").read_text() == "brief — the agent's, untracked\n" + + # --------------------------------------------------------------------------- # Both migration tracks: columns + the backfill (pull on where auto-sync is on) # --------------------------------------------------------------------------- From 96dddec0018710af802a492e81d4b1a88d76191a Mon Sep 17 00:00:00 2001 From: trinity-ability <309458136+trinity-ability@users.noreply.github.com> Date: Fri, 2 Oct 2026 21:03:40 +0100 Subject: [PATCH 8/8] =?UTF-8?q?merge-train:=20narrow=20the=20"never=20disc?= =?UTF-8?q?ards=20local=20work"=20claims=20to=20what=20=5Fsafe=5Fto=5Frese?= =?UTF-8?q?t=20guarantees=20(#3021)=20=E2=80=94=20mechanical,=20per=20the?= =?UTF-8?q?=20merge-train=20note=20on=20the=20PR?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The undo paths no longer reset over a registered execution, but writes outside the process registry (Files API, docker exec, web terminal) made during the integrate window are not protected, as the author's e6cb8b5b7 note concedes. Wording only: two git.py docstrings, the auto_sync.py comment, requirements/github.md and the GitSyncSettingsPanel.vue copy. Co-Authored-By: Claude Opus 5.5 --- docker/base-image/agent_server/auto_sync.py | 4 ++-- docker/base-image/agent_server/routers/git.py | 13 ++++++++----- docs/memory/requirements/github.md | 7 +++++-- .../src/components/GitSyncSettingsPanel.vue | 6 ++++-- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/docker/base-image/agent_server/auto_sync.py b/docker/base-image/agent_server/auto_sync.py index 1ad19f149f..6ab2f6821a 100644 --- a/docker/base-image/agent_server/auto_sync.py +++ b/docker/base-image/agent_server/auto_sync.py @@ -184,8 +184,8 @@ async def run_one_cycle(client: httpx.AsyncClient, home: Path, run_once) -> Opti # trinity-enterprise#703 — the pull cycle (invariant G3: human and fleet work # reaches the agent within a bound). A second loop beside the push loop, gated # per cycle on the owner's `pull_sync_enabled`; the cycle itself is -# `routers/git.py::_run_pull_once` (never under a running execution, never -# discards local work). +# `routers/git.py::_run_pull_once` (never starts under a running execution, +# never resets over a registered one). # --------------------------------------------------------------------------- _last_pull_resolved: Optional[bool] = None diff --git a/docker/base-image/agent_server/routers/git.py b/docker/base-image/agent_server/routers/git.py index 442f78da82..d61bdd99c7 100644 --- a/docker/base-image/agent_server/routers/git.py +++ b/docker/base-image/agent_server/routers/git.py @@ -1177,9 +1177,10 @@ def git(*args, timeout=60): def _integrate_remote(home_dir: Path, branch: str, ahead: int) -> Optional[str]: - """Bring `origin/` into the checked-out branch without ever losing - local work: a fast-forward when nothing is committed locally, a rebase - (aborted on conflict) when something is. See `_with_stash`.""" + """Bring `origin/` into the checked-out branch, putting uncommitted + edits back (or naming the stash): a fast-forward when nothing is committed + locally, a rebase (aborted on conflict) when something is. See + `_with_stash` / `_safe_to_reset` for what an undo does not protect.""" def step() -> Optional[str]: if ahead == 0: ff = run_registered(["git", "merge", "--ff-only", f"origin/{branch}"], @@ -1254,8 +1255,10 @@ def _run_pull_once(home_dir: Path) -> Dict: sync-state.json happens under that lock). - Stale lock litter is reaped first, as the push cycle does: a pull-only agent has no push cycle to do it. - - Refuses to run over unmerged paths. Local work is never discarded; a - failure is recorded, not resolved. + - Refuses to run over unmerged paths. An undo never resets over a + registered execution (`_safe_to_reset`); writes outside the process + registry (Files API, docker exec, web terminal) made during the + integrate window are not protected. A failure is recorded, not resolved. `behind_after_pull` is the checked-out branch's own lag behind `origin/`, not its lag behind `main`; `behind_main` in the git diff --git a/docs/memory/requirements/github.md b/docs/memory/requirements/github.md index d6fe4a7bfc..0c4ba95a66 100644 --- a/docs/memory/requirements/github.md +++ b/docs/memory/requirements/github.md @@ -884,8 +884,11 @@ - a `trinity/*` working branch also merges `main` in, so human pushes to `main` arrive (a merge — the branch is already pushed; conflict aborted) - uncommitted edits stashed and re-applied explicitly - - never discards local work: a conflict is aborted or undone, and recorded; - an edit that could not be put back is named as kept in `git stash` + - does not undo a pull over a registered execution (`_safe_to_reset`): a + conflict is aborted or undone, and recorded; an edit that could not be put + back is named as kept in `git stash`. Writes outside the process registry + (Files API, docker exec, web terminal) during the integrate window are not + protected - never STARTS while an execution is in flight or queued, nor over unmerged paths (and the push cycle never commits them). The gate is check-then-act: admission does not wait for a pull, so a turn accepted during the integrate diff --git a/src/frontend/src/components/GitSyncSettingsPanel.vue b/src/frontend/src/components/GitSyncSettingsPanel.vue index ce7f83632c..2ebc3c3bf9 100644 --- a/src/frontend/src/components/GitSyncSettingsPanel.vue +++ b/src/frontend/src/components/GitSyncSettingsPanel.vue @@ -73,8 +73,10 @@

Brings in edits people and other agents push to the repository, and merges main into a working branch. A pull never starts while the - agent is working or has a turn queued, and never discards its own - changes. With auto-sync on, each push also rebases onto GitHub first. + agent is working or has a turn queued, and a conflicting pull is + undone with the agent's edits put back. Edits made outside agent + turns (uploads, terminal) while a pull runs are not protected. With + auto-sync on, each push also rebases onto GitHub first.