Skip to content

feat: branch ownership enforcement (S7, #382) - #396

Merged
vybe merged 7 commits into
Abilityai:mainfrom
AndriiPasternak31:feature/382-branch-ownership
Apr 19, 2026
Merged

vybe merged 7 commits into
Abilityai:mainfrom
AndriiPasternak31:feature/382-branch-ownership

Conversation

@AndriiPasternak31

Copy link
Copy Markdown
Contributor

Summary

Enforces the "working branches are per-instance-only" invariant for GitHub-native
agents at four layers, so two agents can no longer silently clobber each other's
state when they end up bound to the same (repo, branch) tuple (the
2026-04-17 alpaca incident).

  • Layer 0 — consolidated reservation: single
    reserve_and_generate_instance_id(agent_name, github_repo, ...) helper in
    services/git_service.py that generates a UUID, probes the remote with
    git ls-remote, inserts the agent_git_config row under the new UNIQUE
    index, and retries on either collision up to 5 times. Replaces the three
    ad-hoc generate_instance_id() call sites in crud.py, routers/git.py,
    and the internal initialize_git_in_container path (now deprecated behind
    a warning log).
  • Layer 2 — DB uniqueness: partial
    UNIQUE(github_repo, working_branch) WHERE source_mode = 0 on
    agent_git_config. Source-mode agents share branches intentionally, so the
    predicate excludes them. New migration agent_git_config_branch_ownership
    refuses to install the index when pre-existing duplicates are found,
    printing every offending row so an operator can rebind one of them before
    re-running — never auto-deletes.
  • Layer 3 — push-time guard: docker/base-image/agent_server/routers/git.py
    now pushes with git push --force-with-lease=<branch>:<expected-sha> where
    the expected SHA is the remote value last observed at fetch (persisted to
    ~/.trinity/last-remote-sha/<branch>). When the lease is rejected we
    return HTTP 409 with X-Conflict-Type: branch_ownership_collision and
    append a structured alert entry to ~/.trinity/operator-queue.json so
    the Operating Room surfaces the collision instead of the agent believing
    its push succeeded.

Closes #382
Refs #381

Test Plan

  • bash tests/git-sync/test_p5_branch_ownership.sh — pure-git Tier-1 repro
    demonstrating that --force-with-lease rejects the losing push with
    stale info (exit 1) while the first push succeeds.
  • pytest tests/git-sync/test_s7_reserve_instance_id.py -v — 12 unit
    tests covering the helper (retry, max-retries, DB rollback), the partial
    UNIQUE index (source_mode=0 rejects, source_mode=1 allows, cross-repo
    allowed), and the migration pre-flight (detects duplicates, aborts with
    actionable error, installs index on clean DB).
  • Manual: stand up two agents on the same GitHub repo and confirm the
    Layer 0 reservation prevents duplicate branch assignment.
  • Manual: force a lease mismatch (push from outside Trinity, then sync)
    and confirm a collision card appears in the Operating Room.

Out of scope (deferred to separate issues)

Reviewer notes

  • The create_working_branch=True path inside initialize_git_in_container
    is kept for backwards compat but emits a deprecation warning; no in-repo
    callers hit it after this PR.
  • The migration fails loudly on pre-existing duplicates by design — per the
    proposal's "Deploy as warnings first, fix duplicates, then flip the
    constraint" rollout plan, operators must resolve duplicates before the
    constraint takes effect.
  • Expected conflict with Reset-preserve-state operation (S3) #384 on git_service.py (S3 touches the same file
    for the reset-preserve-state endpoint).

AndriiPasternak31 pushed a commit to AndriiPasternak31/trinity that referenced this pull request Apr 19, 2026
…tion (Abilityai#382)

Reflects the Layer 0 reservation helper and Layer 2 partial UNIQUE
constraint introduced by PR Abilityai#396:

- New `reserve_and_generate_instance_id` (atomic UUID + ls-remote probe
  + DB insert under partial UNIQUE, retries 5x).
- Three legacy `generate_instance_id()` call sites consolidated to
  reserve-before-create with rollback.
- Partial UNIQUE on `agent_git_config(github_repo, working_branch)
  WHERE source_mode = 0` + migration with duplicate pre-flight that
  refuses to install on existing duplicates.
- Sequence diagram split into Phase 4a (reserve loop) / 4b (container
  init); Phase 5 retitled.

Layer 3 push-time guard documented separately in github-sync.md.

Refs Abilityai#382 Abilityai#381
AndriiPasternak31 pushed a commit to AndriiPasternak31/trinity that referenced this pull request Apr 19, 2026
…tyai#382)

Documents the four-layer S7 enforcement landed in PR Abilityai#396 — reservation
helper + partial UNIQUE + force-with-lease + collision alert — alongside
the existing GitHub Sync / Repo Initialization sections.

Refs Abilityai#382 Abilityai#381

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via /validate-pr — 4-layer defense (reserve + ls-remote + partial UNIQUE + force-with-lease) addresses the 2026-04-17 alpaca silent-clobber incident. Tests, rollback paths, and docs all in order.

ap-31 added 7 commits April 19, 2026 09:38
Two complementary tests written red-first per TDD:

- tests/git-sync/test_p5_branch_ownership.sh — pure-git Tier-1 repro.
  Two agents land on the same working branch, observe the remote SHA
  at fetch time, then race to push with --force-with-lease. The
  losing side must exit non-zero with "stale info". Adapted from
  /tmp/trinity-repro/p5_silent_clobber.sh and p5_fix_verified.sh.

- tests/git-sync/test_s7_reserve_instance_id.py — pytest suite.
  Covers the new reserve_and_generate_instance_id helper (retries,
  max-retries exhaustion, DB rollback), the partial UNIQUE index on
  agent_git_config(github_repo, working_branch) WHERE source_mode = 0,
  and the migration pre-flight that refuses to install the index when
  existing duplicates are present.

Local conftest overrides the top-level tests/conftest.py backend-login
fixtures so the unit tests run without a live backend.

Refs Abilityai#382
…ilityai#382)

Adds UNIQUE(github_repo, working_branch) WHERE source_mode = 0 to
agent_git_config so two working-branch agents can no longer be bound
to the same (repo, branch) pair. Source-mode agents intentionally
share branches (every reader pointing at main), so the partial
predicate excludes them.

Migration pre-flight:
- _find_duplicate_working_branches scans for existing duplicates.
- _migrate_agent_git_config_branch_ownership refuses to create the
  index when duplicates are found, printing and logging each
  offending row so the operator can rebind one of the colliding
  agents to a fresh working branch before re-running. Never
  auto-deletes — that would mask the bug this constraint catches.

Fresh installs get the index from schema.py; existing installs pick
it up via the new migration step.

Refs Abilityai#382
Abilityai#382)

Replaces the three ad-hoc generate_instance_id() call sites with a
single reserve_and_generate_instance_id(agent_name, github_repo, ...)
that atomically:

  1. Generates a UUID and builds the working branch name.
  2. Probes the remote via `git ls-remote --heads --exit-code` so a
     name that already exists on GitHub is never re-used (Layer 1).
  3. Inserts the agent_git_config row under the partial UNIQUE index
     introduced by the companion migration (Layer 2).
  4. Retries on either collision up to MAX_INSTANCE_ID_RETRIES (5),
     then raises RuntimeError rather than silently picking a shared
     branch.

Call site changes:

- src/backend/services/agent_service/crud.py: reserve BEFORE
  container creation; drop the duplicate db.create_git_config call
  that previously ran after container boot; roll back the reservation
  on any later failure so retries can claim a fresh branch.
- src/backend/routers/git.py::initialize_github_sync: reserve BEFORE
  initialize_git_in_container; pass the reserved branch in and use
  create_working_branch=False so the helper doesn't generate its own;
  roll back on failure.
- src/backend/services/git_service.py::initialize_git_in_container:
  add `working_branch` kwarg for the pre-reserved path; deprecate
  create_working_branch=True with a warning log (kept for legacy
  callers until they're all migrated).

The plain generate_instance_id / generate_working_branch helpers are
kept but documented as internal — new code must go through
reserve_and_generate_instance_id.

Refs Abilityai#382
…ns (Abilityai#382)

S7 Layer 3 — defense in depth for the force-push path.

- Replace `git push --force` with
  `git push --force-with-lease=<branch>:<expected-sha>`. The expected
  SHA is the remote value we observed at the last successful fetch;
  if another instance wrote to the branch since, the lease is stale
  and the push is rejected cleanly with "stale info" instead of
  silently clobbering the peer's state (2026-04-17 alpaca incident).

- After every successful `git fetch`, persist the observed remote
  SHA to `~/.trinity/last-remote-sha/<branch>`. The push path reads
  this as the lease. Failure to persist is logged but never raised —
  worst case we fall back to the unparameterized
  `--force-with-lease`, which uses the remote-tracking ref as the
  expected-sha.

- When the lease is rejected we append a structured `alert` entry to
  `~/.trinity/operator-queue.json` so the backend's operator_queue
  sync service picks it up on the next poll and surfaces the
  collision in the Operating Room. The agent learns it lost the
  race instead of believing its push succeeded.

Returns HTTP 409 with `X-Conflict-Type: branch_ownership_collision`
on lease rejection so the frontend can distinguish it from the
existing `push_rejected` / `merge_conflict` cases.

Refs Abilityai#382
…tion (Abilityai#382)

Reflects the Layer 0 reservation helper and Layer 2 partial UNIQUE
constraint introduced by PR Abilityai#396:

- New `reserve_and_generate_instance_id` (atomic UUID + ls-remote probe
  + DB insert under partial UNIQUE, retries 5x).
- Three legacy `generate_instance_id()` call sites consolidated to
  reserve-before-create with rollback.
- Partial UNIQUE on `agent_git_config(github_repo, working_branch)
  WHERE source_mode = 0` + migration with duplicate pre-flight that
  refuses to install on existing duplicates.
- Sequence diagram split into Phase 4a (reserve loop) / 4b (container
  init); Phase 5 retitled.

Layer 3 push-time guard documented separately in github-sync.md.

Refs Abilityai#382 Abilityai#381
…nc (Abilityai#382)

Reflects the agent-server changes in commit e2d8402:

- `git push --force` → `git push --force-with-lease=<branch>:<expected-sha>`
- Last-observed remote SHA persisted to `~/.trinity/last-remote-sha/<branch>`
  on every successful fetch and read as the lease on push.
- Lease rejection returns 409 with `X-Conflict-Type:
  branch_ownership_collision` and appends a structured alert to
  `~/.trinity/operator-queue.json` so the Operating Room surfaces the
  collision instead of the agent believing its push succeeded.
- 2026-04-17 alpaca-vybe-live incident cited as motivation; regression
  at tests/git-sync/test_p5_branch_ownership.sh.

Layers 0/2 (reservation + partial UNIQUE) documented in
github-repo-initialization.md.

Refs Abilityai#382 Abilityai#381
…tyai#382)

Documents the four-layer S7 enforcement landed in PR Abilityai#396 — reservation
helper + partial UNIQUE + force-with-lease + collision alert — alongside
the existing GitHub Sync / Repo Initialization sections.

Refs Abilityai#382 Abilityai#381
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Branch ownership enforcement (S7)

3 participants